search

CensoredUsername / dynasm-rs

A dynasm-like tool for rust.
https://censoredusername.github.io/dynasm-rs/language/index.html
Mozilla Public License 2.0
705 stars 52 forks source link

Run cargo fmt #76

Open stevefan1999-personal opened 1 year ago

stevefan1999-personal commented 1 year ago

The change set in #67 looks very long and I don't like that. I realized most of the code are not formatted correctly and as such I blended in cargo fmt result there. Let's make sure the change set is clean first

CensoredUsername commented 1 year ago

Hey there,

There's a lot of nice cleanup changes in there, but cargo fmt also messes up the structure / bloats some code significantly. I feel like this needs a manual pass (and/or some use of rustfmt_skip) to not end up making some parts significantly more annoying to read.

stevefan1999-personal commented 1 year ago

@CensoredUsername I think we need to find a compromise: we need to rather set an agreement upon the rustfmt configs, so that we can have still have a standard to agree on while being less invasive on code changes.

CensoredUsername commented 1 year ago

That makes sense. It's slightly complicated by the fact that I've never used rustfmt on this project, so I do not have a rustfmt config to share here. That said, I really don't have problems with most of the reformatting, but rustfmt's insistence to unroll long enum declarations unnecessary, and its prospensity to remove vertically aligned spacing where it really adds readability is a problem. If just those sections were rustfmt_skip exempted I'd be fine with this changeset. I'll try provide some pointers to what I mean below.

vext01 commented 3 months ago

FWIW, I accidentally ran cargo fmt when doing #92, not realising that we don't use auto-formatting in this repo. I had to then pick my changes apart from the unrelated formatting changes.

Personally I like rustfmt. I think there is worth in everyone being on the same page WRT formatting, but I also respect upstream's decision to choose whether to use it or not.

If we are not using it, I wonder if there's a way to prevent cargo fmt from doing anything so that others don't make the same mistake as I did?

CensoredUsername commented 3 months ago

@vext01 I've just not used it in this repo before, as I didn't feel the need for it when it was mostly me using it. If people want to use it I'm fine with it (see this pull), but there's some places where cargo fmt makes the code significantly harder to read, and I'd like those to be addressed instead of people just blanket running cargo fmt, not looking at how it affects the output, and adding exemptions for where it is needed.

I wrote down some notes on where it felt necessary to me in the requested changes above if you're interested.