Theseus
Theseus copied to clipboard
Run `rustfmt` / `cargo fmt` on source code
Many files have inconsistent formatting which can make it harder to understand the code. Running rustfmt on all files would ensure that the formatting is consistent.
Do you want this, Kevin? I've noticed you've got a lot of careful manual formatting, vertically aligning declarations and match blocks and whatnot. I'm a huge fan of rustfmt
but it will sometimes make code look worse.
An in-between option would be to add #[rustfmt::skip]
to preserve the existing formatting in places rustfmt
really mangles. Of course, doing that would take a good bit of manual scrutiny.
Also, are you happy with the default Rust style guide formatting? If you want to customize anything in rustfmt.toml
best to do that upfront...
I mentioned some of my thoughts here: https://github.com/theseus-os/Theseus/pull/530#issuecomment-1154547570
I'm not opposed to it, but we would indeed need to do some heavy customization of rustfmt before letting it loose on the entire code base. Some of our older crates would definitely benefit from formatting fixes. Unfortunately I don't necessarily have the time to devote to studying and customizing rustfmt, so in #530 I've proposed a similar strategy to satisfying clippy: incrementally apply it to groups of crates until we're satisfied with its behavior.
I have tried getting vscode's settings.json "editor.formatOnSave": true
to work with the suggestion in https://github.com/theseus-os/Theseus/pull/530#issuecomment-1155969831, but that might have to wait for a while: https://github.com/rust-lang/rust-analyzer/issues/10826.
I also tried
"rust-analyzer.rustfmt.extraArgs": [ "--config-path ." ],
so you don't have to ;)
For now I did this: #637
@hecatia-elegua ah, I completely missed your comment above, hence my confusion in #637.
Disclaimer: i haven't looked at rustfmt in a while, so my understanding of its features may be out of date.
I'd vote to start with the crates in applications/
and kernel/
, since those are the main first-party components of Theseus.
The first and easiest thing we need to enforce is a maximum line width (column count). Let's start with a soft limit of 100 columns and a hard limit of 110 columns.
Another big concern with rustfmt is that it seems to eagerly split statements across multiple lines, e.g.,
let result = me.iter().filter(...).find().next();
becomes
let result = me
.iter()
.filter(...)
.find()
.next();
This is okay when each statement is long, but when the statements are short (let result = me.iter().next();
) it becomes tedious and less easy to read. That's the main issue I have, and it seems like chain_width
is one of the relevant settings there.
Another thing I've noticed in PRs that have run rustfmt
is that it destroys manual vertical alignment of variable/const/static definitions. Hopefully a setting is available to preserve that.
Finally, in general I prefer many small PRs over one big PR as it's easier to review that way (and easier to handle merge conflicts). So, perhaps a good way to go about this is to submit a PR that adds one rustfmt setting and includes changes to one file as an example, and then once we've settled on the specifics of that file's changed formatting, then we can apply that one change to other files. Then we'll incrementally add more rustfmt settings in future PRs, and also set up a CI action to ensure new code changes abide by those formatting rules. We've done that with clippy and it has been relatively painless so far.
Formatting imports would be an improvement too, as they're kind of messy and inconsistent. I like the following options:
-
imports_granularity
:Crate
-
group_imports
:StdExternalCrate
Also it looks like the use_small_heuristics
= Max
would address most of my complaints about verbose line splitting.