https://pantsbuild.org/ logo
Join Slack
Powered by
What's the reason that pants uses 2-spaces per tab...
# development
g
What's the reason that pants uses 2-spaces per tab for Rust? I realize this is a holy war in some circles; but I've never once before worked in a Rust repository using 2 spaces. And for me, it severly impacts readability of the code, especially with the fairly dense coding style.
a
IIRC it was effectively an arbitrary choice back in the day.
g
I see! Do you know if it's been discussed since? (On a completely unrelated note; I just saw this PR from you: https://github.com/pantsbuild/pants/pull/12586 -- I realize it's been a long while, but do you know if that mtls support still works? I'm trying to add mTLS for remote-cache, but the upstream barfs at me and fails to verify the client-cert. Unsure if it's something I'm doing wrong in the setup or if it's gotten rotten...)
a
I would expect the rust code to still work - I'm not sure it ever got wired up to python
But shouldn't be too fiddly to do
Do you know if it's been discussed since?
I don't think so, no... Personally, I'd be fine with a change as long as it's done cleanly in one commit, but it may be disruptive to some folks 🙂
g
I've done the wiring all ready, following the pattern you established. Will have to take another peek if there's something I missed... even hard coding the key and cert fails so a bit confused.
Re formatting, I'll think about a formal proposal then and see how more people feel.
a
Strange - I'll test out an 
fs_util
at head later this week and see if anything jumps out 🙂
❤️ 1
b
It does indeed look like the first 'real' rust commit uses 2 spaces: https://github.com/pantsbuild/pants/commit/40c214aa1c044a02b5290aa3ec7cc9570dbe24ad
w
i would be tentatively supportive of any change that made our 
rustfmt
config smaller
g
There's only one line, so I reckon a 100% size reduction should have your support then 😉
w
when we introduced the first rust code, we did not have 
rustfmt
in place, so when @average-vr-56795 introduced 
rustfmt
, he did so with a config that would reduce churn
heh. yes.
(i was thinking of our clippy config i suppose)
g
That one we can also discuss. 😛 But it's easier to fix both as a user and globally... indentation not so much.
a
https://github.com/rust-lang/cargo/pull/12648 should make the clippy situation much cleaner
One entry in the workspace Cargo.toml (and possibly one line per other Cargo.toml, I don't remember how it ended up), rather than a many-line prelude in every crate
g
I think my main complaint so far tbh is that the lints block compilation completely, as opposed to only blocking clippy/CI.
a
They do? I thought they only blocked running 
cargo clippy
...?
g
Nope! If you use pants-from-source (
../pants/pants
from adjacent repos f.ex.) a single unused variable will stop it from running. Maybe that's just that script
a
Aha unused variables are separate from clippy
(Both are valid complaints!)
g
Yeah, maybe I'm sloppily calling the linting (in general) clippy 😛 A lot of those extra flags we keep in .cargo/config.toml instead. Not sure if there's a strong difference.
Here f.ex. in one of the libraries I maintain: https://github.com/EmbarkStudios/cervo/blob/main/.cargo/config.toml
a
The main issue with 
.cargo/config.toml
files is cargo chooses to use them based on your working directory rather than their location relative to the code
So e.g. if we had 
pants/rc/rust/engine/.cargo/config.toml
and you built in 
pants/
, the config wouldn't get applied
But the config question will 100% be solved by Cargo.toml 
[lints]
section
The question remains though - do we want to, and is there a nice way to, have iterative development not treat warnings as errors
w
it would be great for 
cargo clippy
to evolve into 
cargo lint
, and for it to expose warnings. but i don’t imagine that it does enough compilation to actually reproduce all warnings that the compiler might emit
g
Wrote down with some arguments for changing it: https://github.com/pantsbuild/pants/issues/19892 -- I'm not sure how to reach consensus around this kind of proposal, or where it goes from here. The work itself is just deleting 
rustfmt.toml
, running 
cargo fmt
and PRing the whole repo back again 😛
👍 1
a
I'm in favour - two suggestions I'd make: 1. In your PR and commit message doing the reformatting, tell people how to easily rebase past your change (https://github.com/pantsbuild/pants/pull/9157 has an example from last time we did a big reformatting) 2. Maybe set up the commit as ignored for `git annotate`: https://www.stefanjudis.com/today-i-learned/how-to-exclude-commits-from-git-blame/
b
Nice! Hm, it looks like brace-style macros in statement position e.g. 
fn foo() { bar! { ...} }
don't get formatted by default, so lots of 
tokio::select!
and similar calls will be left with 2 spaces. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=30c00d97fbca5ffb336e4d884ce40c32 (Tools > Rustfmt) I wonder if there's a way to convince Rustfmt to format them too
g
Does it work on nightly, do you know? Maybe with some flags? We reformat our codebase with nightly to get early access to gated features. For example, let-else formatting was behind a gate for a long while before getting stabilized.
a
I don't believe there's any opt-in for macro formatting at the moment, no
2 Views
Open in Slack
PreviousNext
Powered by