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

Use Rustfmt Nightly #2365

Open
laralove143 opened this issue Jul 31, 2024 · 6 comments
Open

Use Rustfmt Nightly #2365

laralove143 opened this issue Jul 31, 2024 · 6 comments
Labels
c-all Affects all crates or the project as a whole

Comments

@laralove143
Copy link
Member

Rustfmt nightly has been pretty stable for me and it introduces rules that we're trying to apply manually, such as imports_granularity, so I think we could use it on the nightly channel to reduce inconsistencies and make PRs easier to make.

Here's a sample rustfmt.toml we could use that aligns with Rust's conventions while enabling nightly features.

@laralove143 laralove143 added the c-all Affects all crates or the project as a whole label Jul 31, 2024
@Erk-
Copy link
Member

Erk- commented Jul 31, 2024

I have been rather annoyed that I had to use it every time I develop on Songbird. Since my editor/rust-analyzer wants to do it one way and the CI wants it in a different. So personally I would not be in favour of this.

@vilgotf
Copy link
Member

vilgotf commented Jul 31, 2024

I too would not like to require nightly rustfmt during development, but it would be nice to run CI with such a ruleset (instead of us manually spotting formatting issues). #1857 introduced something similar for doc examples

@BlackHoleFox
Copy link
Member

for another comment similar to Erk's take: Trading off the style consistency of vast swaths of the Rust ecosystem for new/interested contributors in exchange for some specific, minor rules seems like it goes in the wrong direction, imo.

@laralove143
Copy link
Member Author

I too would not like to require nightly rustfmt during development, but it would be nice to run CI with such a ruleset (instead of us manually spotting formatting issues). #1857 introduced something similar for doc examples

we could also do this, in that case though the flow would go like user pushes changes -> formatting ci fails -> user pushes formatting chanes -> formatting ci maybe succeeds, for prs involving many commits, wouldnt this introduce many formatting commits? this is fine since we squash and merge but i wonder if it'd be the best DX for contributors

if we go that route, i'd suggest recommending contributors to use rustfmt nightly but they'd still have the option to not use it and fix formatting issues post-push

@AEnterprise
Copy link
Member

having people push to then have to manually fix them because we use a different linter also sounds like a very frustrating contributor experience. for the docs it kinda makes sense as having those be invalid will cause confusion to users. but formatting is just style and very personal with varying opinions. Yes consistency is nice but it shouldn't be at the cost of significant development complications

and if it's just a recommendation but we're not going to enforce it then it might as well not be there as it'll just create a weird situation where some code follows it stricly, and then some doesn't

@laralove143
Copy link
Member Author

laralove143 commented Aug 10, 2024

i agree reporting formatting issues in ci would lead to a bad contribution experience

as for your second point, i'm not saying that we don't try to follow certain formatting, i'm saying that we should and pr reviews should aim to check whether this style is followed, but for contributors to follow our formatting style we can recommend them to use rustfmt nightly and if they don't want to, they can format their code manually, we can also recommend a way for developers to check the format without applying fixes so that they don't have to push and wait to see if the ci passes

with the style provided to the contributors, it wouldn't affect the contributors' experience if we added a check for formatting in our ci, as it would just automate reviewers having to check the format of the code in prs

while i agree that dx comes before consistency, they don't have to be exclusive to each other, we can find ways to improve consistency without worsening dx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-all Affects all crates or the project as a whole
Projects
None yet
Development

No branches or pull requests

5 participants