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

Add .rustfmt.toml to repo + format code #269

Closed
Ben-Lichtman opened this issue May 6, 2023 · 7 comments · Fixed by #270
Closed

Add .rustfmt.toml to repo + format code #269

Ben-Lichtman opened this issue May 6, 2023 · 7 comments · Fixed by #270

Comments

@Ben-Lichtman
Copy link

Ben-Lichtman commented May 6, 2023

I'd like to contribute to this project, however it seems like the style is mostly ad-hoc and rustfmt is not being used for the most part?

I propose adding a .rustfmt.toml to the root of the project to ensure that contribution style is consistent, then running cargo fmt on the repo to format the code.

I've tried to come up with a .rustfmt.toml which is minimally imactful while still providing some benefits:

edition = "2021"
hard_tabs = true
match_block_trailing_comma = true
control_brace_style = "ClosingNextLine"

Unfortunately this still leads to a large number of changes due to many small adjustments.

More settings can be found here

One other potential issue is the lack of a setting for preserving aligned spaces such as

const SOME_LONG_NAME: i32       = 5;
const SHORT: i32                = 6;

This patten is found in a few places, especially src/image.rs
In this case if you want to preserve the nice alignment there are a few options:

  • Add #[rustfmt::ignore] annotations to each line that you want to preserve formatting
  • Move the aligned variables into an inline module and mark it as #[rustfmt::ignore]
  • Add #![rustfmt::ignore] to the top of the file to avoid formatting the whole thing
@Ben-Lichtman
Copy link
Author

The other option is to disable rustfmt project-wide with disable_all_formatting in the .rustfmt.toml, though I'd suggest that it's better to pick a suboptimal style rather than ad-hoc style chosen by contributors

@Ben-Lichtman
Copy link
Author

I would PR this, but it would be difficult to review due to the large number of changes, so I'll leave it up to the maintainer to push a version with cargo fmt run on it

@Ben-Lichtman Ben-Lichtman changed the title Add .rustfmt.toml to repo Add .rustfmt.toml to repo + format code May 6, 2023
@CasualX
Copy link
Owner

CasualX commented May 7, 2023

Thanks for your consideration! I've been working on implementing a rustfmt policy that I can stomach.

I'm not a fan of autoformatters in general as they tend to make everything look consistently ugly.
But with some okay settings, a nightly rustfmt, refactoring some code and liberal application of rustfmt::skip I think I can make it work.

I'll try to get a PR up in a few more hours.

@CasualX
Copy link
Owner

CasualX commented May 7, 2023

Good lord I just noticed rustfmt rewrote everything to lf which conflicts with my windows checkout and usage of autocrlf setting... I'll try to keep going and get the repo to cleanly pass rustfmt but rustfmt is just not ready for actual usage...

@Ben-Lichtman
Copy link
Author

Ben-Lichtman commented May 7, 2023

You can adjust this with the newline_style setting.

Looks like the default is "auto" though which tries to use whatever that file already uses, so your file probably already had a LF present.

@CasualX
Copy link
Owner

CasualX commented May 7, 2023

I can confirm that this is not the case, all files changed by cargo +nightly fmt (idk if nightly matters here) are changed from crlf to lf. Look like it is an open bug in rustfmt: rust-lang/rustfmt#4097

Some git magic later it's all fixed but these are the kind of rough edges that confirm my biases against rustfmt 😛

@CasualX CasualX mentioned this issue May 7, 2023
CasualX added a commit that referenced this issue May 10, 2023
@CasualX
Copy link
Owner

CasualX commented May 10, 2023

It took some time but it's there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants