Skip to content
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

rustfmt: run it on doc examples too #1368

Merged
merged 3 commits into from
Nov 11, 2019

Conversation

dianpopa
Copy link
Contributor

@dianpopa dianpopa commented Nov 1, 2019

Ran rustfmt from the nightly build with format_code_in_doc_comments enabled.

Signed-off-by: Diana Popa [email protected]

Reason for This PR

Format the doc code examples -> better looking Firecracker

Description of Changes

[Author TODO: add description of changes.]

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria. Where there are two options, keep one.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • Either this PR is linked to an issue, or, the reason for this PR is
    clearly provided.
  • The description of changes is clear and encompassing.
  • Either no docs need to be updated as part of this PR, or, the required
    doc changes are included in this PR. Docs in scope are all *.md files
    located either in the repository root, or in the docs/ directory.
  • Either no code has been touched, or, code-level documentation for touched
    code is included in this PR.
  • Either no API changes are included in this PR, or, the API changes are
    reflected in firecracker/swagger.yaml.
  • Either the changes in this PR have no user impact, or, the changes in
    this PR have user impact and have been added to the CHANGELOG.md file.

Copy link
Member

@andreeaflorescu andreeaflorescu left a comment

Choose a reason for hiding this comment

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

I noticed that the indentation is a bit wierd for all the code examples where examples are built in functions. Can we either uncomment the function names or remove the functions? I did some research when I started working on rust-vmm, and most of the crates out there don't use functions that return results in examples without calling them. I assume that the reason is the code examples can fail silently with this approach. I think it is easier to keep the documentation up to date if the doc tests fail.

@dianpopa
Copy link
Contributor Author

dianpopa commented Nov 4, 2019

I noticed that the indentation is a bit wierd for all the code examples where examples are built in functions. Can we either uncomment the function names or remove the functions? I did some research when I started working on rust-vmm, and most of the crates out there don't use functions that return results in examples without calling them. I assume that the reason is the code examples can fail silently with this approach. I think it is easier to keep the documentation up to date if the doc tests fail.

I did this. I do not understand what you mean by weird indentation. I just ran cargo fmt --all in order to format the doc tests, I did not do anything manual (and not planning to do). I just thought it would not hurt to have this in our codebase so that when that feature is stable we have less files to update.

@dianpopa dianpopa self-assigned this Nov 4, 2019
@andreeaflorescu
Copy link
Member

Here is an example where the lines have a weird indentation:

    /// # use memory_model::{GuestAddress, GuestMemory, MemoryMapping};
    /// # fn test_write_u64() -> Result<(), ()> {
    /// #   let start_addr = GuestAddress(0x1000);
    /// #   let mut gm = GuestMemory::new(&vec![(start_addr, 0x400)]).map_err(|_| ())?;
    /// let buf = &mut [0u8; 16];
    /// let res = gm
    ///     .read_slice_at_addr(buf, GuestAddress(0x200))
    ///     .map_err(|_| ())?;
    /// assert_eq!(16, res);
    /// Ok(())
    /// # }

Even though the line let buf = &mut [0u8; 16] is part of the function test_write_u64, only the first two lines of the function actually use the 2 spaces indentation. I assume that this is because the function declaration is actually commented out.

and format the code afterwards.

Signed-off-by: Diana Popa <[email protected]>
@dianpopa
Copy link
Contributor Author

dianpopa commented Nov 8, 2019

Here is an example where the lines have a weird indentation:

    /// # use memory_model::{GuestAddress, GuestMemory, MemoryMapping};
    /// # fn test_write_u64() -> Result<(), ()> {
    /// #   let start_addr = GuestAddress(0x1000);
    /// #   let mut gm = GuestMemory::new(&vec![(start_addr, 0x400)]).map_err(|_| ())?;
    /// let buf = &mut [0u8; 16];
    /// let res = gm
    ///     .read_slice_at_addr(buf, GuestAddress(0x200))
    ///     .map_err(|_| ())?;
    /// assert_eq!(16, res);
    /// Ok(())
    /// # }

Even though the line let buf = &mut [0u8; 16] is part of the function test_write_u64, only the first two lines of the function actually use the 2 spaces indentation. I assume that this is because the function declaration is actually commented out.

I fixed those in another commit. Take a look.

@dianpopa dianpopa merged commit caddf19 into firecracker-microvm:master Nov 11, 2019
@dianpopa dianpopa deleted the little_fixes_2 branch November 11, 2019 09:49
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