Add coding style tool #681

h3ndrk · 2024-01-14T07:32:53Z

Introduced Changes

This PR adds the "checky" tool that iterates over all Rust files in our repository and checks them for the following coding style rules:

mod/use rules
- mod must appear before use
- use must be in the following order: std, extern crates, crate
- all use items of the same category must be ordered: pub, crate, pub(...), ``
- all these major and minor ordering groups must be separated with empty lines
empty line rules
- enum, fn, impl, struct, and trait must be separated by empty lines
- fn within impl must be separated by empty lines

ToDo / Known Issues

/dev/null

Ideas for Next Iterations (Not This PR)

Be that restrictive with empty lines in mod/use rules?
Add --fix support
Integrate in pepsi clippy e.g. by moving the code in an extra crate and use it from Pepsi
Add this tool to our CI
Add rules for: Add WrapErr, do not capitalize

How to Test

Run cargo run --manifest-path=tools/checky/Cargo.toml from the root directory of the repository.

schmidma · 2024-01-15T09:32:59Z

for sorting granularity of use statements: https://rust-lang.github.io/rustfmt/?version=v1.7.0&search=#group_imports

regarding order of mod and use statements, the rust style guide differs to your proposal: https://doc.rust-lang.org/beta/style-guide/items.html#items

Put imports before module declarations.

for blank lines between items: https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#blank_lines_lower_bound

oleflb · 2024-01-17T15:34:23Z

We talked about this in the devmeeting and decided we don't want to maintain another tool since rustfmt can take care of the things you listed. If you want, you can create a correct rustfmt.toml file that enforces these behaviors.

h3ndrk · 2024-01-17T21:36:32Z

You are proposing to use rustfmt instead. These features are not available in stable rustfmt.

for sorting granularity of use statements: https://rust-lang.github.io/rustfmt/?version=v1.7.0&search=#group_imports

This feature is sadly unstable. It also does not implement "all use items of the same category must be ordered: pub, crate, pub(...), ...".

regarding order of mod and use statements, the rust style guide differs to your proposal: https://doc.rust-lang.org/beta/style-guide/items.html#items

We can change that to comply with the style guide, that is no problem. For me consistency is the important thing. This style guide seems to be Beta, so not official?

for blank lines between items: https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#blank_lines_lower_bound

This feature is sadly also unstable. Hopefully we don't get the empty lines in the beginning of functions?

We talked about this in the devmeeting and decided we don't want to maintain another tool since rustfmt can take care of the things you listed. If you want, you can create a correct rustfmt.toml file that enforces these behaviors.

Oh, that was a fast decision. I hoped to argue for my solution but I was late to the meeting due to work as I mentioned earlier.

So do I conclude correctly that we would be fine to require our developers to use an unstable toolchain to use the unstable features? Until now, this was a no-go? 🤔

oleflb · 2024-02-14T21:02:19Z

After todays discussion I'm more open to this PR, since I thought previously this PR would introduce another external tool which I'd not have been a fan of. Since @h3ndrk understandably wants formatting rules for this use cases which are not covered by rustfmt yet, I think we can use this tool until the respective checks are stable in rustfmt. Despite that I would like to hear the opinion @schmidma, since he originally also was not a fan of this extra tool.

schmidma

I left many ideas for improvement. Feel free to discuss ;)

tools/checky/src/checks/mod.rs

tools/checky/src/main.rs

tools/checky/src/checks/empty_lines.rs

tools/checky/src/checks/mod_use_order.rs

schmidma · 2024-05-05T18:36:13Z

This branch has conflicts ;)

h3ndrk · 2024-05-09T17:18:37Z

@schmidma Please have a look again. The tool is largely rewritten to comply with our coding standards. It also features much better error reports thanks to a third-party crate.

The mod_use_order check is currently disabled because we should discuss the order that we want and this needs to be implemented then.

schmidma · 2024-05-23T16:53:29Z

This branch has conflicts and is also not passing the CI 😉

schmidma

some remarks and suggestions

schmidma · 2024-05-23T16:57:14Z

tools/checky/src/main.rs

+#[derive(Parser, Debug)]
+struct Arguments {
+    paths: Vec<PathBuf>,
+}


Please provide docstrings (///) such that clap automatically generates proper help pages

schmidma · 2024-05-23T16:59:38Z

tools/checky/src/main.rs

+
+#[derive(Parser, Debug)]
+struct Arguments {
+    paths: Vec<PathBuf>,


Can you somehow tell clap that the user should provide at least one path? currently, the tool also successfully completes when no paths are given

schmidma · 2024-05-23T17:01:39Z

tools/checky/src/main.rs

+            if !entry
+                .path()
+                .extension()
+                .map(|extension| extension == "rs")
+                .unwrap_or_default()
+            {
+                continue;
+            }


If I provide a single non-rust file, I'd expect this tool to fail. Currently it silently runs to completion.

e.g.

cargo run --manifest-path tools/checky/Cargo.toml -- Cargo.toml

Do you have a suggestion how to implement that without adding a lot of complexity in this file?

schmidma · 2024-05-23T17:04:26Z

tools/checky/src/main.rs

+            let file = RustFile::try_parse(entry.path())
+                .wrap_err_with(|| format!("failed to parse {}", entry.path().display()))?;
+
+            for report in check(&file) {


Suggested change

for report in check(&file) {

let reports = check(&file);

for report in reports {

Just because this is where the magic happens...

schmidma · 2024-05-23T17:06:58Z

tools/checky/src/checks.rs

+pub fn check(file: &RustFile) -> Vec<Report> {
+    let mut reports = Vec::new();
+    reports.extend(empty_lines::check(file));
+    // reports.extend(mod_use_order::check(file));


Why is it disabled? If it is dead code, we should remove it for now?

We need to first decide on the way how we want to order that. I forgot what we decided in some Dev-Meeting. So maybe you need to decide it again. Then we can enable the check and fix all wrong mod-use-orders.

schmidma · 2024-05-23T17:10:31Z

tools/checky/src/checks.rs

+
+pub fn check(file: &RustFile) -> Vec<Report> {
+    let mut reports = Vec::new();
+    reports.extend(empty_lines::check(file));


Suggestion for the check API, as some check may want to have configuration parameters:

pub trait Check { fn check(&self) -> impl IntoIterator<Item = Report>; } // [...] let empty_lines = EmptyLines::new(/*possible setup here, read from toml etc./*); reports.extend(empty_lines.check(file));

or maybe even additionally

pub trait Check { fn check(&self, context: &mut ReportContext) -> impl IntoIterator<Item = Report>; }

which hides the allocation in Vec<Report> or even directly prints them. This way the check only calls something like context.report(...)

What do you think?

I tried quickly but this requires to think a bit about lifetimes etc. which is too much for me in this PR. Either someone takes this over or we could do this in a next iteration.

schmidma · 2024-05-23T17:16:03Z

tools/checky/src/rust_file.rs

+        file.read_to_string(&mut buffer).wrap_err_with(|| {
+            format!("failed to read file {} to string", path.as_ref().display())
+        })?;
+        let path = path.as_ref().to_path_buf();


I'd expect this to be cloned at the end of the function and not here, and then reborrowed

Moved the .to_path_buf() at the end. Did you mean this? 🤔

schmidma · 2024-05-23T17:18:00Z

tools/checky/src/checks/empty_lines.rs

+) -> Option<Report<'a>> {
+    let line_before = before.span().end().line;
+    let line_after = after.span().start().line;
+    if line_before + 1 == line_after {


Suggested change

if line_before + 1 == line_after {

let something_with_subsequent = line_before + 1 == line_after;

if something_with_subsequent {

Named the documentation variable is_subsequent.

schmidma · 2024-05-23T17:18:48Z

tools/checky/src/checks/empty_lines.rs

+        let Some(line_before) = file.source.line(line_before - 1) else {
+            panic!("line {line_before} does not exist");
+        };
+        let Some(line_after) = file.source.line(line_after - 1) else {
+            panic!("line {line_after} does not exist");
+        };


These are .expect() calls, are they?

Now the format string is created regardless if there was a None but maybe that is fine. .expect() only takes &str.

schmidma · 2024-05-23T17:19:48Z

tools/checky/src/checks/empty_lines.rs

+    } else {
+        None
+    }


refactor for early returns

schmidma · 2024-07-31T13:31:44Z

Do we want to revive this? Or do you want to pass this to someone else @h3ndrk ?

h3ndrk · 2024-07-31T16:46:56Z

Feel free to take this over. I fixed some things but the mod-use-order check still needs some work.

h3ndrk enabled auto-merge January 14, 2024 09:28

h3ndrk force-pushed the checky branch from 2d6d627 to 6689e31 Compare January 14, 2024 09:28

h3ndrk added tools:CI tools:Tooling Related to pepsi et.al. labels Jan 14, 2024

schmidma requested changes Apr 5, 2024

View reviewed changes

schmidma self-assigned this Apr 5, 2024

h3ndrk force-pushed the checky branch 3 times, most recently from 12bfb08 to 7fda14a Compare May 9, 2024 17:15

h3ndrk force-pushed the checky branch from 7fda14a to 3b18202 Compare May 9, 2024 17:55

schmidma requested changes May 23, 2024

View reviewed changes

schluis added the is:Postponed label Jul 15, 2024

schmidma removed the is:Postponed label Jul 21, 2024

h3ndrk added 10 commits July 21, 2024 14:01

Add checky tool

70766df

Add empty lines checking

b26252d

Implement file walking

21d980c

Format

a9d0b62

Update Cargo.lock

459abc9

Change to new module source filenames

159cb81

Rewrite checky tool

d1ac3a9

Apply fixes

c209403

Disable mod/use check temporarily until decided how we want to sort

271a793

Review fixes

f858669

h3ndrk force-pushed the checky branch from 3b18202 to f858669 Compare July 21, 2024 12:30

oleflb unassigned schmidma Aug 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add coding style tool #681

Add coding style tool #681

h3ndrk commented Jan 14, 2024

schmidma commented Jan 15, 2024

oleflb commented Jan 17, 2024

h3ndrk commented Jan 17, 2024

oleflb commented Feb 14, 2024 •

edited

Loading

schmidma left a comment

schmidma commented May 5, 2024

h3ndrk commented May 9, 2024

schmidma commented May 23, 2024

schmidma left a comment

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma May 23, 2024

h3ndrk Jul 21, 2024

schmidma commented Jul 31, 2024

h3ndrk commented Jul 31, 2024

	for report in check(&file) {
	let reports = check(&file);
	for report in reports {

	if line_before + 1 == line_after {
	let something_with_subsequent = line_before + 1 == line_after;
	if something_with_subsequent {

Add coding style tool #681

Are you sure you want to change the base?

Add coding style tool #681

Conversation

h3ndrk commented Jan 14, 2024

Introduced Changes

ToDo / Known Issues

Ideas for Next Iterations (Not This PR)

How to Test

schmidma commented Jan 15, 2024

oleflb commented Jan 17, 2024

h3ndrk commented Jan 17, 2024

oleflb commented Feb 14, 2024 • edited Loading

schmidma left a comment

Choose a reason for hiding this comment

schmidma commented May 5, 2024

h3ndrk commented May 9, 2024

schmidma commented May 23, 2024

schmidma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidma commented Jul 31, 2024

h3ndrk commented Jul 31, 2024

oleflb commented Feb 14, 2024 •

edited

Loading