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

install rustfmt from nightly #696

Merged
merged 6 commits into from
Dec 7, 2023

Conversation

OmarTawfik
Copy link
Collaborator

allowing us to use any experimental features/flags in .rustfmt.toml. I created one for now with all the default flags. We can review/update them in future PRs as needed:

rustfmt  --print-config default .rustfmt.toml

Nightly is only used for formatting. Other cargo commands to build/check/test/run source code are still using the default stable toolchain.

I additionally updated the docs for infra scripts, running clippy as part of infra check cargo, since running it separately duplicates some of the work, and its lints are as important as cargo check. It also decouples its fixes/suggestions from linters like rustfmt, so it can format any applied changes.

Copy link

changeset-bot bot commented Dec 5, 2023

⚠️ No Changeset found

Latest commit: 60429f9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@OmarTawfik OmarTawfik marked this pull request as ready for review December 5, 2023 11:23
@OmarTawfik OmarTawfik requested a review from a team as a code owner December 5, 2023 11:23
@Xanewok
Copy link
Contributor

Xanewok commented Dec 5, 2023

To be honest, as much as I'd love the import grouping being readily and automatically applied, I'd rather not depend on the nightly rustfmt, as it's a moving target - we now have to worry about two toolchain versions (one being updated daily) and it'd be harder to reproduce the formatting suggested by rustfmt; I've done this a couple of times already and I'd not repeat that.
I'd probably prefer if we spent the effort on trying to push the relevant options closer to stabilisation upstream in rustfmt ❤️

Maybe a good balance would be to have a dedicated helper command that runs the nightly rustfmt with the import grouping specified as a CLI param? We wouldn't block on this in CI but we could still reap the benefits of clearer imports until the option is stabilised.

Also, I'm not a fan of explicitly specifying every option - the style and defaults will probably change over the years/editions and I'd rather we explicitly specify options that we ourselves prefer or want to enforce (e.g. max_width) and rely on the defaults as much as possible; after all, having more knobs is not always better and I'd prefer if we're being vanilla/standard to reduce maintenance/cognitive burden.

Lastly, I'm somewhat torn on always running Clippy when we intend to run cargo check. Maybe I'm biased but I've also encountered some Clippy-specific build failures in the past and I'd probably prefer if we had a clean cargo check as an option (clippy lints are just helpful, whereas rustc says what's correct and valid code). Then again, maybe it's a lot more stable nowadays and we're using stable Clippy here; we're also using it as our go-to checking command in the IDE, so I'm open about it.

(..) so it can format any applied changes.

The lint suite relied on the order of the subcommands, so we did run clippy before running rustfmt. However, I agree that it was rather implicit and unreliable, so anything that makes it more explicit and more reliable is better 👍

@OmarTawfik
Copy link
Collaborator Author

@Xanewok thank you for taking a look!

I'd rather not depend on the nightly rustfmt, as it's a moving target - we now have to worry about two toolchain versions (one being updated daily) and it'd be harder to reproduce the formatting suggested by rustfmt; I've done this a couple of times already and I'd not repeat that.

Good point. How about using a fixed nightly version? would that help? we can choose to upgrade the specific nightly/stable versions on demand. But otherwise, everything will be stable. I updated the PR to use nightly-2023-12-01 specifically.

Also, I'm not a fan of explicitly specifying every option

I agree. This tries to reduce future noise by "fixing" the default values, in case an unstable feature changed its defaults later. But I also that is no longer a concern if we use a fixed nightly version. We can worry about that only during upgrade. I will keep this file empty for now.

Maybe it's a lot more stable nowadays and we're using stable Clippy here; we're also using it as our go-to checking command in the IDE, so I'm open about it.

I think the same, mainly because we are already running it by default in the IDE. It doesn't make sense to fix one without the other. And if running them together means faster type checking, I think it is a bigger win.

@OmarTawfik OmarTawfik force-pushed the rustfmt-nightly branch 2 times, most recently from b348088 to d2210c3 Compare December 6, 2023 12:36
allowing us to use any experimental features/flags in `.rustfmt.toml`. I created one for now with all the default flags. We can review/update them in future PRs as needed:

```sh
rustfmt  --print-config default .rustfmt.toml
```

Nightly is only used for formatting. Other cargo commands to build/check/test/run source code are still using the default stable toolchain.

I additionally updated the docs for `infra` scripts, running `clippy` as part of `infra check cargo`, since running it separately duplicates some of the work, and its lints are as important as `cargo check`. It also decouples its fixes/suggestions from linters like `rustfmt`, so it can format any applied changes.
Copy link
Contributor

@Xanewok Xanewok left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Left some nitpicks, otherwise it's good to go.

crates/infra/cli/src/commands/setup/cargo/mod.rs Outdated Show resolved Hide resolved
crates/infra/cli/src/commands/mod.rs Outdated Show resolved Hide resolved
crates/infra/cli/src/commands/lint/mod.rs Show resolved Hide resolved
@OmarTawfik OmarTawfik mentioned this pull request Dec 7, 2023
@OmarTawfik OmarTawfik added this pull request to the merge queue Dec 7, 2023
@Xanewok
Copy link
Contributor

Xanewok commented Dec 7, 2023

Looks great, thanks for the work ❤️

Merged via the queue into NomicFoundation:main with commit 9a78865 Dec 7, 2023
1 check passed
@OmarTawfik OmarTawfik deleted the rustfmt-nightly branch December 7, 2023 10:40
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.

2 participants