Argyle-Software / kyber

A rust implementation of the Kyber post-quantum KEM
https://docs.rs/pqc_kyber/
Apache License 2.0
163 stars 37 forks source link

Modify: Enforce code style with rustfmt #71

Closed mberry closed 10 months ago

mberry commented 1 year ago

Format: 2 space tabs, max_width = 80, brace_style = "AlwaysNextLine" Multiple large array constants have been skipped to preserve cleanliness Update: contributing and readme to show usage of unstable cargo fmt

Closes #63 Closes #45

mberry commented 1 year ago

This is a massive diff, so I'm going to leave it pending for quite a while. If anyone wants to bikeshed style choices I'm more than happy to discuss, there's probably a few other rustfmt options that could make things nicer, but this config mostly follows in line with the crates style.

faern commented 1 year ago

Really good to enforce a style. Makes it so much easier to contribute when you can just run cargo fmt and know that the code adheres to what the project wants :+1: :+1:

If you want to bikeshed, I'm not a huge fan of any of the chosen non-default options :see_no_evil:. I have a personal preference for the defaults, plus it makes all rust code look the same, which is nice. I always use rustfmt in all my projects, but I very rarely have a rustfmt.toml. Just use defaults :sparkles:.

I personally find it much easier to read the code when it's not too much/often written vertically. I think 80 chars is too little for any language almost. The 100 default is really good IMHO.

2 spaces for indent? This will make your code look different from the vast majority of other Rust code. And this is of course 100% a subjective opinion. I find it harder to visually find the start and end of blocks when the indentation is barely noticeable (which I think 2 spaces is).

brace_style = "AlwaysNextLine" really makes the code smell like C++ to me. Also 100% personal preference. But I find it using way too much vertical space and looking really odd. But again, depends on what you are used to.

I'm mostly arguing that the default rustfmt style will look the most familiar the most number of Rust developers. At work, when we pick up a new language and we bring up style I always say "Find the standard/most used formatter for the language and format with its default settings". This way the code will blend in in the ecosystem and moving between projects in the same ecosystem is smoother and feels more natural.

mberry commented 1 year ago

Understand that it is essentially the reasoning behind cargo fmt refusing to stabilise most of their features to make it the "norm". Personally I prefer 2 spaces and 80 char soft / 100 char hard limit, but happy to simply accept the cargo fmt defaults and setup git pre/post-commit hooks for my personal use, this seems a decent outcome that doesn't force contributors to subscribe to my preferences.
Should also be a basic check in CI too for any PR's

faern commented 1 year ago

Should of course be enforced in CI indeed. Otherwise it will go out of sync instantly. Thats what the --check option is for 👍

mberry commented 10 months ago

Let's try this again with #95