Skip to content

fs: code improvement #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 10, 2024
Merged

fs: code improvement #4

merged 3 commits into from
Apr 10, 2024

Conversation

phip1611
Copy link
Contributor

Hi there, nice project! :)

I found some room for a code improvement.

PS: You defined some UEFI protocols such as TCPv4ServiceBindingProtocol that
we'd love to see in the uefi crate (specifically the uefi-raw crate).
If you have some time, don't hesitate to upstream them, similar as done in
rust-osdev/uefi-rs#1118

PPS: @nicholasbishop look what cool project I found that uses uefi-rs

This increases build reproducibility and developer experience.
1.) the uefi-crate provides utilities for conversions from &str
    to Path
2.) .expect() messages should have a "should" wording. Please refer
     to [0] for more information

[0]: https://doc.rust-lang.org/std/error/index.html#common-message-styles
@codyd51
Copy link
Owner

codyd51 commented Apr 10, 2024

Hi Philipp! First of all, great name.

I'm really glad you had a look at the project! I wanted to give a heads up to the uefi-rs folks, but didn't see any sort of forum when checking out the Github project. Thank you for reaching out, and for creating the great infrastructure!

The code changes are also much appreciated. I've tested things and everything seems to work fine. I will go ahead and merge your PR.

I'm glad you mentioned about upstreaming the TCP protocol, as I also had this in mind (and I found the TCP protocol quite tricky to get working well, so hopefully others can get use out of the work too). However, I will be starting a new job next week, and my current understanding is that I won't be able to work on side projects or contribute to open source. Sadly, I would not currently be able to do more than a 'drive-by' PR in which I copy in the relevant portions, and would not be able to do extra polish that might be necessary to bring it up to your project's guidelines. This isn't how I'd prefer to submit contributions, but if it would still be useful to you, and if you would be willing to take the PR to the finish line, I would gratefully submit a PR.

Let me know your thoughts!

@codyd51 codyd51 merged commit 2f54779 into codyd51:main Apr 10, 2024
@phip1611 phip1611 deleted the dev branch April 11, 2024 09:15
@phip1611
Copy link
Contributor Author

Hi Philipp! First of all, great name.

( ͡° ͜ʖ ͡°)

I'm really glad you had a look at the project! I wanted to give a heads up to the uefi-rs folks, but didn't see any sort of forum when checking out the Github project. Thank you for reaching out, and for creating the great infrastructure!

We have an Zulip instance: https://rust-osdev.zulipchat.com/

But to be fair: It is rarely used and also not very advertised.

However, I will be starting a new job next week

great!

and my current understanding is that I won't be able to work on side projects or contribute to open source.

Oh, that's a sad. Fingers crossed that you can convince your new employer about the relevance of paid engagement in free open source projects.

Sadly, I would not currently be able to do more than a 'drive-by' PR in which I copy in the relevant portions, and would not be able to do extra polish that might be necessary to bring it up to your project's guidelines.

I think that once you set up a basic MR, @nicholasbishop and I can do the rest of the work. What do you think, Nicholas?

@nicholasbishop
Copy link

Very neat project :)

I think that once you set up a basic MR, @nicholasbishop and I can do the rest of the work. What do you think, Nicholas?

Yep! A drive-by PR is totally fine by me, we can take care of finishing up the work.

@codyd51
Copy link
Owner

codyd51 commented Apr 14, 2024

Thank you both very much for the support! I have gone ahead and opened a PR here.

I have ported my TCP client code in addition to the protocol definitions, but if you think it's appropriate to merge only the definitions that's completely fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants