Skip to content

documentation and code improvements for Status, Error, and read() #556

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 27, 2022
Merged

documentation and code improvements for Status, Error, and read() #556

merged 2 commits into from
Nov 27, 2022

Conversation

phip1611
Copy link
Member

@phip1611 phip1611 commented Nov 11, 2022 •
edited
Loading

This MR improves the documentation of the low-level read method of the file system protocol on a regular file or a directory.

Following our discussions, I changed the scope to:

documentation and code improvements for Status, Error, and read()

Checklist

  • Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
  • Update the changelog (if necessary)

@phip1611 phip1611 added enhancement documentation Issues related to the documentation labels Nov 11, 2022
@phip1611 phip1611 force-pushed the read-entry-return-code branch 2 times, most recently from 7f4df87 to f5c23e7 Compare November 11, 2022 21:44
@phip1611
Copy link
Member Author

phip1611 commented Nov 11, 2022 •
edited
Loading

It might be a little unusual for the current code-base to copy&paste UEFI spec documentation into the documentation of a method, but I think we should do it this way for "rather complex" functionality. What do you think?

@phip1611 phip1611 force-pushed the read-entry-return-code branch 3 times, most recently from 8bd739d to 5c9e1a2 Compare November 11, 2022 21:55
nicholasbishop
nicholasbishop requested changes Nov 11, 2022
View reviewed changes
@phip1611 phip1611 force-pushed the read-entry-return-code branch from 5c9e1a2 to 7a189bf Compare November 12, 2022 08:53
@phip1611 phip1611 changed the title RegularFile/Directory::read: Improved Return Type and Documentation Documentation Improvements for RegularFile/Directory::read Nov 12, 2022
@phip1611 phip1611 force-pushed the read-entry-return-code branch from 7a189bf to 76ab34b Compare November 12, 2022 08:58
@phip1611 phip1611 force-pushed the read-entry-return-code branch 2 times, most recently from f127399 to 51de27b Compare November 12, 2022 09:26
phip1611
phip1611 commented Nov 12, 2022
View reviewed changes
nicholasbishop
nicholasbishop requested changes Nov 12, 2022
View reviewed changes
@phip1611 phip1611 force-pushed the read-entry-return-code branch from 51de27b to e528e6d Compare November 24, 2022 08:23
@phip1611 phip1611 changed the title Documentation Improvements for RegularFile/Directory::read documentation and code improvements for Status, Error, and read() Nov 24, 2022
@phip1611
Copy link
Member Author

phip1611 commented Nov 24, 2022 •
edited
Loading

Hi @nicholasbishop
I changed the scope of the MR following our discussions. You are right, we need to stay consistent. I think that the current changes better reflect my initial understanding problems. Hopefully, those changes also help others. Are we good to go now?

@phip1611 phip1611 force-pushed the read-entry-return-code branch 2 times, most recently from 80fde34 to 2eac5d5 Compare November 24, 2022 08:27
phip1611
phip1611 commented Nov 24, 2022
View reviewed changes
uefi/src/proto/media/file/mod.rs
@@ -332,6 +332,19 @@ pub(super) struct FileImpl {
) -> Status,
close: extern "efiapi" fn(this: &mut FileImpl) -> Status,
delete: extern "efiapi" fn(this: &mut FileImpl) -> Status,
/// # Read from Regular Files
Copy link
Member Author

@phip1611 phip1611 Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a copy of relevant parts of the UEFI spec.

@phip1611 phip1611 force-pushed the read-entry-return-code branch from 2eac5d5 to 6882a5e Compare November 24, 2022 08:29
@nicholasbishop
Copy link
Member

Added a couple more suggestions, but otherwise lgtm!

@phip1611 phip1611 force-pushed the read-entry-return-code branch from 6882a5e to 21b8290 Compare November 27, 2022 09:06
@phip1611 phip1611 force-pushed the read-entry-return-code branch from 21b8290 to d295e8d Compare November 27, 2022 09:09
@phip1611 phip1611 merged commit 235bc84 into rust-osdev:main Nov 27, 2022
@phip1611 phip1611 deleted the read-entry-return-code branch November 27, 2022 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicholasbishop nicholasbishop nicholasbishop requested changes

Assignees
No one assigned
Labels
documentation Issues related to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phip1611 @nicholasbishop