Import formatting in code

[wyfo]

There is currently no import formatting rules in the codebase; adding import formatting could improve code maintainability, reducing the potential conflicts between contributions, as well improving code esthetics (for what it's worth, I know that this point is quite subjective).

rustfmt support import formatting, but the configurations options are still unstable. There are mainly two options: import granularity and group imports.
Also, contrary to other Rust component, it's possible to use unstable rustfmt options, but only using CLI, so formatting imports can be on stable Rust using for example --config "unstable_features=true,imports_granularity=Crate,group_imports=StdExternalCrate".

The downside of enabling import formatting is that it requires for contributors to setup their development environment with these options, or face a red CI. Adding a pre-commit hook can greatly help for that, and the fact that stable Rust can be used, so it doesn't requires contributor to download nightly Rust, makes the pre-commit hook worth considering.

[davidhewitt]

I'm open to exploring this. Is the rustfmt configuration for VSCode straightforward?

I have been wondering about a pre-commit hook for a while anyway, a lot of the time I forget to run clippy or silly things like that.

We could potentially use https://pre-commit.ci/ to auto-fix formatting on PRs if it's a major problem, the local hooks are probably good enough though.

[wyfo]

Is the rustfmt configuration for VSCode straightforward?

I don't use VSCode on my side, but you should be able to configure it simply using:

"rust-analyzer.rustfmt.extraArgs": [
    "--config",
    "unstable_features=true,imports_granularity=Crate,group_imports=StdExternalCrate"
]

I have been wondering about a pre-commit hook for a while anyway, a lot of the time I forget to run clippy or silly things like that.

Running clippy in pre-commit hook can be annoying, because on large project like PyO3, it may add too much latency to the commit process. That's why I would only put cargo fmt in the pre-commit check, and maybe some other fast hooks like typos.

The question is then: which import formatting rules do we chose?
On my projects, I always use imports_granularity=Crate,group_imports=StdExternalCrate, but I know some people preferring imports_granularity=Module for example.

[Icxolu]

The rust-analyzer extension also has these options build in (under the rust-analyzer.imports.* keys), so no need to juggle with manual extra args. (I personally prefer module granularity and no grouping) Having granularity.enforce disabled worked quite well to just integrate into the style of each file so far.

If we decide to enforce a particular style, we should probably also (re)consider gitignoreing for example .vscode so that these can be easily adjusted as a workspace setting.

[mejrs]

I'm missing how this needs to be a commit hook. Can't we make a rustfmt.toml?

We could potentially use https://pre-commit.ci/ to auto-fix formatting on PRs

I do not like this, just make it fail CI please.

[Icxolu]

Can't we make a rustfmt.toml?

That would be the easiest and probably the preferred solution, but the problem is that these options are unstable and thus can only be used on the nightly channel when specified via rustfmt.toml (kinda weird that it's allowed via the CLI then IMO).

We could potentially use https://pre-commit.ci/ to auto-fix formatting on PRs

I do not like this, just make it fail CI please.

I agree here, I'd much rather fix it myself.

If we want to enforce a style, fmt-CI and documentation should be enough.

[davidhewitt]

👍

I guess that even if these options are unstable, the worst case (as we are using stable toolchain) is that we have to fixup formatting at the same time as fixing UI tests when the compiler bump is released.

So it sounds like the concensus is lets move forwards here by updating CI and documenting how developers can configure their IDE (as part of Contributing.md).

Overall I am always happy to have to not think about formatting myself and defer to tools, so import formatting is a good thing IMO.

[davidhewitt]

For the options, personally I would vote for imports_granularity=Module,group_imports=StdExternalCrate.

I think Module grouping is closer to what we already have and also is IMO slighty easier to read than Crate grouping due to splitting imports from different depths onto separate lines.

[mejrs]

I like Module as well.

lets move forwards here by updating CI and documenting how developers can configure their IDE (as part of Contributing.md).

I'd actually prefer we don't do that. People use a variety of IDE's (or not at all; I don't use rust-analyzer for example), and I feel this might cause friction for people new to contributing. 9 out of 10 new contributors won't read it or miss it, and run into this.

IMO the simplest option is to just do a one time format with the Module setting, and not test/format it after.

[mejrs]

Also, this will cause merge conflicts with a bunch of (big) open prs. I'd like to have those merged before we proceed with a format pr.

[Icxolu]

I'd actually prefer we don't do that. People use a variety of IDE's (or not at all; I don't use rust-analyzer for example), and I feel this might cause friction for people new to contributing. 9 out of 10 new contributors won't read it or miss it, and run into this.

Hmm, without enforcing it I don't really see the benefit, it would just create friction. Without CI the style will drift apart again over time, so we might as well just leave it as is. IMO we should either commit here and get the benefits (consistency, easier reviewing, less conflicts) or we don't and stay as is (lower bar for new people)

[wyfo]

I'd actually prefer we don't do that. People use a variety of IDE's (or not at all; I don't use rust-analyzer for example), and I feel this might cause friction for people new to contributing. 9 out of 10 new contributors won't read it or miss it, and run into this.

Formatting is already checked in the CI, it's just about changing formatting configuration. If people have a failed job in their CI, they should already check the job output to see what is wrong.

By the way, we could modify the nox -s rustfmt output when the issue is about import formatting, to make it clear in the CI logs that the contributor should format import with the right rustfmt command. It could be done by running cargo fmt without the formatting options first, then run it with formatting options and catch the error to add a message about import formatting and link to Contributing.md.

[mejrs]

Formatting is already checked in the CI,

We check formatting; we don't check whether the user has formatted with some particular flag, The latter is what this PR proposes. I'm fine with the latter if the error message for it is good enough, i.e. it is not confusing to people who haven't memorized the contribution guidelines.