CasualX / pelite

Lightweight, memory-safe, zero-allocation library for reading and navigating PE binaries.
MIT License
290 stars 42 forks source link

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

Closed Ben-Lichtman closed 1 year ago

Ben-Lichtman commented 1 year ago

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:

Ben-Lichtman commented 1 year ago

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 commented 1 year ago

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

CasualX commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/rust-lang/rustfmt/issues/4097

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

CasualX commented 1 year ago

It took some time but it's there