SpectralSequences / sseq

The root repository for the SpectralSequences project.
Apache License 2.0
22 stars 10 forks source link

More restrictive `rustfmt` #146

Closed JoeyBF closed 5 months ago

JoeyBF commented 5 months ago

This is a result of running cargo +nightly fmt on the repository with the following rustfmt.toml:

unstable_features = true
format_code_in_doc_comments = true
format_macro_matchers = true
format_macro_bodies = true
format_strings = true
imports_granularity = "crate"
reorder_impl_items = true
reorder_imports = true
group_imports = "StdExternalCrate"

In the future I plan to use this configuration for my PRs. I feel it's a bit more systematic than the default settings

hoodmane commented 5 months ago

Doesn't running a nightly formatter incur a lot of git blame noise for limited benefit? I would think it's better to stick to stable formatters unless there is a good reason not to.

JoeyBF commented 5 months ago

In theory yes, but almost all of the changes in ext are either reordering imports or spacing things out differently. The changes in chart are just because I wanted to be thorough, running the usual cargo fmt would also produce a large diff.

I'm not suggesting we enforce a nightly formatter, but I personally like using those nightly features and this way I wouldn't worry about my autoformatter messing with the actual contents of a PR

hoodmane commented 5 months ago

Actually the changes do look quite nice.

I'm not suggesting we enforce a nightly formatter,

Can we pin this specific version and enforce it in CI?

JoeyBF commented 5 months ago

Can we pin this specific version and enforce it in CI?

Yeah definitely! Should we change the lint CI so that it runs on stable and nightly-2024-02-09? Right now it runs on stable + beta + nightly. I could include the rustfmt.toml file in the top directory, but that will make stable rustfmt crash.

The issue with running it only on this version of nightly though is that we won't get the updates from stable in the future

hoodmane commented 5 months ago

Should we change the lint CI so that it runs on stable and nightly-2024-02-09?

This sounds reasonable supposing that the new lint and the old lint don't get into fights. But I guess our old approach of not pinning the linter version wasn't causing too much churn?

JoeyBF commented 5 months ago

It never caused me any issue. I think it's pretty rare that nightly ships without rustfmt. I say we just go with nightly then, to save us the issues of linting on stable with a rustfmt.toml that uses unstable features

JoeyBF commented 5 months ago

All good to merge?

hoodmane commented 5 months ago

Yeah looks good to me. Could you rebase it into two commits:

  1. a config change
  2. the diff applied

and then do a non-squash merge? It's pretty hard to find the config changes in the 134 files changed.

JoeyBF commented 5 months ago

Sure thing!

hoodmane commented 5 months ago

Thanks @JoeyBF!