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

staging: add workspace-wide rustfmt lints #552

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

epilys
Copy link
Member

@epilys epilys commented Nov 24, 2023

Summary of the PR

Closes #488

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@epilys
Copy link
Member Author

epilys commented Nov 24, 2023

Making this into a draft PR since CI lacks rustfmt on nightly toolchain

@epilys epilys marked this pull request as draft November 24, 2023 08:28
Copy link
Contributor

@Ablu Ablu left a comment

Choose a reason for hiding this comment

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

Looks like this removes quite a few ambiguous situations with the current rules :)

# Reorder import and extern crate statements alphabetically in groups (a group is separated by a newline). (default value)
reorder_imports = true
# Reorder mod declarations alphabetically in group. (default value)
reorder_modules = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we set these to default values? Any reason why we set them at all then? I think we probably want to follow the overall Rust defaults but only tweak a few options that we find useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought to include them in case the defaults change. They are not unreasonable to have and sometimes rust rolls back on choices. Is it that bad, you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am just worried that nobody will ever review the delta that we got compared to Rust default. Then we are left with code laws that only exist because they happened to be default at some point. IMHO saying "we track Rust defaults, expect these few stricter rules" keeps our rule set simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you're right. We're not able to easily see changes against current defaults so it can confusing. I'll remove them

Copy link
Collaborator

Choose a reason for hiding this comment

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

The convention I tend to see is the defaults are described but commented out.

staging/rustfmt.toml Show resolved Hide resolved
staging/rustfmt.toml Show resolved Hide resolved
@@ -20,7 +20,7 @@
},
{
"test_name": "staging: style",
"command": "cd staging && cargo fmt --all -- --check --config format_code_in_doc_comments=true"
"command": "cd staging && cargo +nightly fmt --all -- --config-path rustfmt.toml --check"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe ping the nightly version number down to prevent a nightly update breaking CI? Or does that already happen elsewhere?

Would it make sense to add an overview of which of the format options are nightly only? Eventually, we would probably want to drop the nightly dependency, right?

Copy link
Member

Choose a reason for hiding this comment

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

I also suggest to leave custom-tests.json as much as possible equal to the one used out of staging (maybe we should change the name).

So my suggestion is to add another file with change, where we can also run it on no-staging crates. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we do want to drop it, I am not familiar with how rustfmt rules get stable though. What would be the best approach you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@stefano-garzarella a separate .json file with just the nightly fmt? is that possible?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, I just need to update buildkite with that file.
Should we also run the same on no-staging crates?

Copy link
Member Author

Choose a reason for hiding this comment

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

@stefano-garzarella

If we add that commit to .git-blame-ignore-revs file, what could be other issues?

(Anyway, for now I'm fine to enable it only for staging)

None, just the amount of lines to review at once. Sometimes rustfmt has bugs and we should be on the lookout in general.

Copy link
Member

Choose a reason for hiding this comment

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

My approach would be to gradually lift these rules everywhere. For example it was in sound only at first. Now it's staging. Then the root workspace in a follow up PR.

Yep, I agree on that!
I'll send a PR to change custom-tests.json to staging-tests.json with a comment to explain that we want to leave it aligned with the rust-vmm-ci tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we add that commit to .git-blame-ignore-revs file, what could be other issues?

I think that file is still opt-in, or did something change?

Copy link
Member

Choose a reason for hiding this comment

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

We can do one commit per rule in root crates then, and at the end add the rustfmt.toml file for all crates. What do you think?

Yep, I agree on that. 1 commit per rule with the parameters described in the commit description.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can do one commit per rule in root crates then, and at the end add the rustfmt.toml file for all crates. What do you think?

Sounds good to me. Except, I would probably add the rules in some rust-vmm wide manner.

@Ablu
Copy link
Contributor

Ablu commented Nov 24, 2023

Ah, sorry. I missed that this is for staging only... So maybe my comments were overly strict...

There's no reason to use 2018, 2021 is the default nowadays.

Switch the versions in the rustfmt files of sound and video while at it.

Signed-off-by: Manos Pitsidianakis <[email protected]>
Both staging crates had the same rustfmt.toml file, let's make it
workspace default.

Signed-off-by: Manos Pitsidianakis <[email protected]>
Add a new test json file with a cargo-fmt check with the nightly
toolchain. This allows to use nightly rustfmt rules in rustfmt.toml.

Signed-off-by: Manos Pitsidianakis <[email protected]>
Add doc comments for each rule and a bunch of new rules.

Signed-off-by: Manos Pitsidianakis <[email protected]>
Add previous formatting commit SHA to .git-blame-ignore-revs

The filename does not seem to be standardised, but must be configured
explicitly.

Note: to use this file always, you must set:

  git config --local blame.ignoreRevsFile .git-blame-ignore-revs

(or --global)

or use git blame with `--ignore-revs-file .git-blame-ignore-revs`

Signed-off-by: Manos Pitsidianakis <[email protected]>
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.

CI: run nightly cargo fmt wiht a custom rustfmt.toml
4 participants