Proposed fix: configuring and formatting project.

[oestradiol]

This Pull Request:

• Adds .editorconfig and rustfmt.toml, which are config files to ensure consistent formatting throughout the whole project;
• Adds the @generated annotation to all generated files + creating cargo alias for cargo fmt to skip those files;
• Configures rustfmt to run with the CI + adds a Problem Matcher to improve output.

#

Hello @sugyan! Good day.

I'm making this PR because I ran into an annoying issue. I routinely use cargo fmt to ensure consistent formatting in my code, but I noticed that the code on atrium-api wasn't properly formatted. Therefore, I added two configuration files that hint to our IDEs and cargo fmt how it should format files. That ensures consistent formatting, which is an overall code improvement. I highly recommend merging this into main, as well as running cargo fmt before any git push, if your IDE doesn't do it by default.

I would love to hear your thoughts on it!

P.S.: If you don't like the current style of formatting, you can look into the definitions for both of these files and we can update them and see how that looks, how is that for you? Also, if I remember correctly, there are ways to override the formatting style for specific folders. Since atrium-api is generated code, it might come in handy!

[sugyan]

Thank you for the pull request!

Yes... much of the atrium-api's source code is generated from lexicon/lexgen and uses the prettyplease crate for its formatting. It would have been ideal to be able to call the rustfmt code from inside codegen, but apparently, that was not possible. For a while, I tried formatting by calling rustfmt from std::process::Command. However, I decided on my current choice because it takes time to format all the files, and contributors may not have the environment to run rustfmt. The processing speed was important, especially in the early stages of development, when we had to rewrite codegen many times and check the success of atrium-api builds through trial and error. https://github.com/sugyan/atrium/pull/80/files#diff-22393f79d3c3c495febd5b5c7f60f2d6d0c9e4ed78256721db40231a634537da

I was not in the habit of applying cargo fmt to the entire code of the project, as I was formatting when saving each file with formatOnSave in VSCode, and I never had any problems with it. So I knew the generated code would have different results than when I used rustfmt, but I never really worried about it because I just didn't have to edit it.

But yes, it is problematic that running cargo fmt causes a diff. It would be good to merge this pull-request and make sure to run cargo fmt before git push.

I've investigated other solutions.

• Is it possible to write #![rustfmt::skip] at the beginning of the file so that the whole file is skipped? Apparently this is unstable and cannot be used.

• rustfmt has a function to skip generated files, so use that. https://github.com/rust-lang/rustfmt/pull/4296 Change the leading comment to // @generated and specify the following in rustfmt.toml:

  unstable_features = true
  format_generated_files = false

  After all, this is also an unstable feature, so running it with cargo +nightly fmt will indeed skip it, but it won't work with cargo fmt... https://rust-lang.github.io/rustfmt/?version=master&search=#format_generated_files

[oestradiol]

I see, so those seem to be the best options, huh?

1. Manually running cargo fmt before each git push, or maybe before every big commit to main (Pull Requests, branch merges, etc.), which seems reasonable;

2. Adding #![rustfmt::skip] to every generated file (unstable feature);

3. Using format_generated_files + inserting @generated in every file.

   • This options is also unstable, however! https://github.com/rust-lang/rustfmt/issues/5080#issuecomment-1013307669 - There seems to be a way to override it without activating nightly. We might as well give it a try.

That being said, I wonder if it would be possible to automate this into the workflow? Something similar to how release-plz works. Doing these "chores" by hand before every merge, PR, or after every generation seems a little bit tough and time-consuming. If there was a way to automate it with Github Actions, that would be perfect.

In particular, I find the 3rd option the most appealing, even if the stable option (passing through command line) ends up not working for some reason, it still seems fairly better than the other options. The @generated annotation is not intrusive, and would fit well in the comment we already have: // @generated - This file is generated by atrium-codegen (...)

Alright, I think we could work this out. Tell me your thoughts, and after testing a few things, we could make a decision. Looking forward to it!

[sugyan]

fmm, // @generated comment and cargo fmt -- --config format_generated_files=false seems to work well in stable. At least the codegen generation comments could be changed to use @generated.

[oestradiol]

Ok! I think it's good to go. Seems like it worked just fine.

The next step would be making github actions to run cargo fmt -- --config format_generated_files=false before every commit. I assume it should be simple?

[sugyan]

Thank you for the update! Basically, pull-requests will be merged after checking the CI results, so I'm wondering if adding cargo fmt --check -- --config format_generated_files=false to rust.yml, for example, would be enough to detect formatting flaws.

Please let me know if you know of any rust projects that might be of help regarding formatting guidelines.

[oestradiol]

I think that's a good idea, yeah! But considering formatting is automatic, maybe there's a way to immediately format the branch while merging? Formatting shouldn't break anything after all, so I don't know if there's a good reason to reject the PR until it's properly formatted, rather than just doing it and merging. That's up to your jurisdiction and preference, though. I think both options work.

About the Rust project you asked, I can't think of anything right now, but I'll see if I can find anything good.

[oestradiol]

Okay, I found one that seems promising. rust-analyzer seems to be very well structured, with both rustfmt.toml and .editorconfig files.

They also seem to have some extra configuration for linting at .github/, here: rust.json.

Github Workflows also seem to be properly configured, their CI includes actions for cargo clippy (lines 113 to 115) and also a rustfmt check (lines 117 to 119). Proper configuration for the RUSTFLAG env (line 18) is also included, with deny for all important lints for the entire CI.

There's also something really cool they did. They're using .cargo/config.toml to create aliases for specific commands, which we definitely should do too for cargo fmt --check -- --config format_generated_files=false!

[sugyan]

Thank you for investigating! The rust-analyzer looks great for reference.

Perhaps .github/rust.json is used to annotate errors output by Actions? As for clippy, I think the report in giraffate/clippy-action is sufficient, but for rustfmt, we could use the same configurations. The rust-analyzer is running cargo fmt -- -check in actions, which seems to fit the policy I wrote before.

As format_generated_files cannot be specified in rustfmt.toml in stable, it seems that the only way to make it work simply with cargo fmt is to use aliases in .cargo/config.toml.

So, the remaining work to be done on this branch is:

• Add .github/rust.json ?
• Add cargo fmt --check ... to rust.yml
• Add .cargo/config.toml for aliases ?

is that correct?

[oestradiol]

Yes, I think so! Those three, plus adding giraffate/clippy-action.

[sugyan]

FYI. clippy-action is already set up in this repository. https://github.com/sugyan/atrium/blob/main/.github/workflows/clippy.yml

[oestradiol]

Oh, right, hahah. Okay!

[oestradiol]

Ok, I have finished the pending tasks. However, before we merge this, I wanted to ask something.

I was thinking maybe it would be better to merge the clippy.yml workflow into the rust.yml one. That way we can configure a single environment that works for both, and I think it makes sense for clippy to run with the rest of the CI.

More specifically, I was thinking of putting it immediately before rustfmt, since if clippy fails, there's no point in checking the formatting of the files. In my opinion, the order should be:

• The code compiles? (cargo build);
• The code is properly linted and well-coded? (cargo clippy);
• If everything is okay, then is the code well-formatted? (cargo fmt --check).

What do you think?

[oestradiol]

Another thing I was thinking is, maybe we should configure global lints for this workspace (although I think it would be better to do this on a separate Pull Request). Something like what I did here.

[sugyan]

It certainly doesn't need to be separated into rust.yml and clippy.yml. Perhaps it was created in a separate file to make sure it worked correctly when adding CI for clippy and was left in place. https://github.com/sugyan/atrium/pull/146 It would be better to merge them.

I've never configured lints in Cargo.toml, but it looks good. However, it seems that the feature only works from 1.74 onwards. https://blog.rust-lang.org/2023/11/16/Rust-1.74.0.html#lint-configuration-through-cargo

ATrium is currently on MSRV 1.70 (background on that is https://github.com/sugyan/atrium/pull/100) and unless this is updated, the lints setting will probably be ignored even if it is.

[oestradiol]

Since they were the author of the mentioned PR (#100), I'd like to call them here to hear their opinion as well, as their input will be valuable. @str4d, I summon you hehe.

I was looking at tokio's MSRV policy and I think it's very reasonable: minimum 6 months, only updating when necessary. Since Rust 1.75 has been out since Dec. 28th 2023, which was ~9 months ago, I think it should be OK to bump our MSRV to 1.75.

That way, we will be able to configure lints, and also remove async-trait from our dependencies.

If you both approve this change, I'll be creating two new issues: one to bump MSRV and remove async-trait, another one to configure the lints.

I'd like to hear what you have to say!

[str4d]

I'm fine with bumping MSRV to 1.75. I target N - 4 as the "most recent MSRV" for my own projects, which is around 6 months or so.

[oestradiol]

All right. Thanks for your input!

Also @sugyan, on another topic, if everything in this PR is OK for you, I think it's ready to be merged 👍🏻

[sugyan]

Thank you both!

OK, let's bump up the MSRV to 1.75. I think we can proceed with just setting up the lints first, and removing async-trait can proceed separately again.

By the way, rustfmt detected a diff in Actions. Is it due to the use_small_heuristics item in rustfmt.toml?

[oestradiol]

@sugyan yes, correct. I configured the option and forgot to run cargo fmt-no-gen, whoops..

I pushed a new commit. Should be good now!

[sugyan]

Thank you. Hmm... I'm used to the default settings, so I personally feel uncomfortable with them, but well, they are also the settings used by rust-analyzer, and I feel that I will get used to them in time.

[oestradiol]

Thanks for being receptive! <3

Yes, I'm also not fully used to them but I honestly think it's better and looks cleaner this way. It's easier to read too, so I decided it was worth a shot. I hope we can both get used to it quick!