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

Can we please have CI check for rustfmt? #270

Open
dan-da opened this issue Dec 20, 2022 · 2 comments
Open

Can we please have CI check for rustfmt? #270

dan-da opened this issue Dec 20, 2022 · 2 comments

Comments

@dan-da
Copy link

dan-da commented Dec 20, 2022

Presently we have a check for clippy errors, but badly formatted code is allowed.

Once some pending PRs have been reviewed+merged, I would be happy to make a PR that runs rustfmt over the entire repo, at which point we could enable a CI check and enforce rustfmt style going forward....

@racs4
Copy link
Contributor

racs4 commented Dec 23, 2022

This was already proposed, but was not considered because some people who already participated more actively in the development of the project did not like the style proposed by cargo fmt.

This is summed to other aspects such as word alignment, which with formatting would be lost, such as

let sum  = 3   + 2;
let sum1 = sum + 2;
let sum2 = sum + 4;

which would lose that "pretty style" if formatted (most code like that was found in the hvm.rs file, which contained the Runtime, Term, Statements, etc; later this file was split, but I believe there is still code like that in the kindelia_core/runtime/mod.rs file and others resulting from this division). Some people used and defended this style, and because of this "discussion" cargo fmt was not used in CI.

@dan-da
Copy link
Author

dan-da commented Dec 24, 2022

Ok, thx for sharing that history! I don't wish to step on any toes, but I will make some counterpoints for sake of discussion and moving ball foward, hopefully constructively.

aspects such as word alignment

Formatting can be preserved within a code block (statement, function, module) using eg #[rustfmt::skip] and files can be skipped with rustfmt.toml.

I understand that skipping at block level is not perfect when one really just wants to skip the next 5 lines or whatever. But the ability to just skip a fn should probably suffice?

Also rustfmt default settings can be overridden with rustfmt.toml which gives some control over rustfmt's style.

still code like that in the kindelia_core/runtime/mod.rs file and others

yeah, so those files could be ignored by rustfmt entirely as a first step.


I totally get the desire to preserve space alignment or other custom formatting for some code that looks better that way.

At the same time, by not using rustfmt, we are losing the benefits of standardized formatting for all the rest (large majority I think) of the code that does not require such.

So I would advocate that we reverse the situation and rustfmt the majority of code and skip the minority that needs custom formatting.

I think this will make kindelia code more accessible for others in the rust ecosystem. And as more contributors submit PRs it is best practice to have a standard coding style, and rustfmt makes that very easy to achieve and enforce.

and what is the alternative? That each contributor submits PRs in whatever style suits them at the time for the entire codebase without fixing up newlines, extra spaces, etc? That seems rather messy...

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

No branches or pull requests

2 participants