chore: Add workflow for running tests/lints/format checks on CI#49
chore: Add workflow for running tests/lints/format checks on CI#49define-null wants to merge 7 commits intomasterfrom
Conversation
.github/workflows/tests.yaml
Outdated
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: dsherret/rust-toolchain-file@v1 | ||
| - run: cargo fmt --all -- --check |
There was a problem hiding this comment.
The problem with formatting is that Eldar used a custom styleguide for this repo, allowing 2 empty lines between the blocks. However, the blank_lines_upper_bound option in rustfmt is still unstable. So applying the formatting at the moment will try to remove extra empty lines in all of the files
There was a problem hiding this comment.
Do we consider unifying the style-guides on the org level or at least standardizing style-guide for the repo so that it can be enforced? Otherwise either no style guide is followed, or it becomes tedious to battle with the tools, that apply default fmt rules.
There was a problem hiding this comment.
Usually we follow the default style-guide in the company. Only this repo has its own one which, unfortunately, can't be enforced at the moment. Actually, maybe enabling unstable features for rustfmt alone is not a bad solution 🤔
There was a problem hiding this comment.
Yes it could be done just on the rustfmt level.
But are there any reasons not to switch to the default org-wide style-guide for this repo?
If you are worrying about tools like git-blame - it's easily solved with:
git config blame.ignoreRevsFile .git-blame-ignore-revs
and .git-blame-ignore-revs file that includes commit refs that did massive reformatting.
There was a problem hiding this comment.
I think the files in this repo are quite long, so I agree with Eldar that two-line separation sometimes looks better. So I would vote for allowing two-line spaces in other repos as well instead of reformatting this one
There was a problem hiding this comment.
I will remove fmt part from this PR, because I don't see that working locally with the current toolchain, cargo fmt uses the formatter from whichever toolchain is active, so we can’t transparently have both:
- cargo clippy to call stable clippy
- cargo fmt to call nightly rustfmt
without impacting the usual cargo ergonomics.
Only this repo has its own one which, unfortunately, can't be enforced at the moment.
I may be misunderstanding something here, but I’m not sure why we couldn’t adjust the formatting configuration directly in this repository. I also suggested a couple of approaches that would avoid disrupting git blame
5f08d15 to
406de6a
Compare
0572f36 to
6aef07a
Compare
What is this PR about?
PR adds workflow with jobs for running clippy, unit tests and format checks. The list of checks for clippy and default fmt rules are left as is to open the discussion rather then necessarily advocate for the particular rules or style settings.