metamath / metamath-knife

Metamath-knife can rapidly verify Metamath proofs, providing strong confidence that the proofs are correct.
Apache License 2.0
26 stars 11 forks source link

Rustfmt and style #18

Closed tirix closed 3 years ago

tirix commented 3 years ago

I'm opening this issue to discuss how to best introduce rustfmt.

I would like to add a cargo fmt -- --check to the Travis CI and enforce some formatting rules. About when to introduce it, I think we will agree that the sooner, the better.

Next is the question of the style to configure. Introducing rustfmt's defaults, will result in one commit with changes in almost all files, but we will comply to the Rust formatting guidelines. Another option may be to tune rustfmt's configuration to adapt to the current style, and minimize changes in the initial rustfmt run.

I probably would do the former (conform to Rust guidelines, no customization), but I wanted to have a discussion before launching it, since I see @digama0 has a custom configuration in mm0-rs (although not used).

digama0 commented 3 years ago

I won't impose my style guide on others, in particular two space indent seems to be contentious although I greatly prefer it. I'm fine with the default settings.

david-a-wheeler commented 3 years ago

I agree on enforcing style rules.

I generally prefer whatever the default is in the language. I personally have a mild preference to 2-space instead of 4-space, but since we want others to join who are probably more used to 4-space, 4-space is probably better long term.

However, it's a pain to merge after reformats, so we need to merge all outstanding work first.

tirix commented 3 years ago

Excellent! I'll move on with a reformat once Clippy passes (I'll just have to assume no one has uncommitted changes).

I did a test run, and as a matter of fact, metamath-knife is currently formatted with 4 spaces, so keeping 4 spaces will greatly minimize changes.

However, the current style seems to be the "Visual" alignment for function arguments and function calls. That's where most of the changes will be if we change to the default, which is "Block".

Besides that, it looks like Rustfmt will mainly break a few long lines and reorder imports, those are not big issues.

digama0 commented 3 years ago

I have found that the best way to do a big formatting run is to just run it and accept the consequences. Looking at what changed just raises blood pressure. I'm on board with using the default style here.

tirix commented 3 years ago

Ok, this issue was just about taking a decision. Decision is taken, so I'm closing it.

david-a-wheeler commented 3 years ago

@tirix - great, thanks so much!