Add Clippy linter step to CICD

[Enselic]

Run the linter on the minimum supported rust version; otherwise we will
get lint warnings for things that require a too high Rust toolchain
version to fix.

Allow the following warnings, since we already have them our code:

• clippy::new-without-default
• clippy::match-bool
• clippy::if_same_then_else

Eventually we should fix these lint issues and then disallow them to
prevent them from coming back in other places.

The clippy args used is recommended here:
https://github.com/rust-lang/rust-clippy#travis-ci

I noticed that my PR #1402 passed CICD checks despite having some lint issues, so I thought it would be a good idea to add a linter step to CICD.

I tested the new step in GitHub Actions over at my fork, and it seems to work. You can check here if you want: https://github.com/Enselic/bat/runs/1467487513?check_suite_focus=true (Please ignore my temp changes to CICD.yml to be able to make the test).

But of course it would be a good idea for you to make sure the new step works when triggered from this repo as well. I think you have to trigger it manually since I don't have permission to trigger new CI/CD steps in this repo (which makes sense for security reasons of course).

(I didn't add anything about this to CHANGELOG.md since the change does not affect the behavior of bat, as touched upon in CONTRIBUTING.md)

[sharkdp]

Thank you for you contribution!

We just recently fixed a few clippy warnings (#1369), but I'm not 100% convinced we should enforce 0 clippy warnings in CI (see #1369 (comment)).

Would this break the build if new clippy warnings are introduced?

As there are more people which seem concerned about this, I'm happy to reconsider.

[sharkdp]

But of course it would be a good idea for you to make sure the new step works when triggered from this repo as well. I think you have to trigger it manually since I don't have permission to trigger new CI/CD steps in this repo (which makes sense for security reasons of course).

No, it will just run.. see the build for this PR.

[Enselic]

First of all, I just want to make it clear that I will of course respect whatever decision you make in this matter.

I also understand and agree that we do not want to scare contributors away over some coding style or matter-of-taste issue.

However:

• A lint-free code base is in general nicer to work with (and contribute to) than a linty code base
• clippy includes a set of checks for incorrect (or likely incorrect) code. I do think it makes sense to enforce at least these clippy checks via CI.
• Even if we do not want to enforce any clippy rules via CI yet, we can still let it run for each push/pr so that the ones that want to write lint-free code conveniently can check and make sure that their PRs are lint free.
• If it turns out that some particular clippy rule is particularly annoying, we can always allow violations of that rule on a case-by-case basis. In other words, the set of enforced rules can be a living thing that we tweak as we go.

The current set of clippy checks can be browsed here: https://rust-lang.github.io/rust-clippy/master/
One can see that the only ones that are Deny by default are the lint checks in the correctness group. So if we were to use the following clippy command: cargo clippy --all-targets --all-features then the build will only fail if a correctness rule is violated. All other lint categories will just result in a warning being printed in the build log. I personally think this is a good middle-ground.

What do you think about the above proposal?

(I will create another PR in the near term to fix the last correctness lint we have)

Would this break the build if new clippy warnings are introduced?

Yes, but

• only if they belong to the correctness category
• since we only run clippy on the MIN_SUPPORTED_RUST_VERSION, I think the risk of that is very low
• If it happens it is easy to temp-disable it by adding an --allow argument the clippy command in CICD.yml

One occasion where we will likely see a big "sudden" set of new lint errors is when we bump MIN_SUPPORTED_RUST_VERSION. Since I suppose that is a task for project maintainers though, I don't see the risk of that scaring contributors away.

No, it will just run.. see the build for this PR.

My bad! I somehow managed to not see it even though I looked for it.

[sharkdp]

Thank you for the detailed answer. I agree with all of your arguments, so let's enable this 👍

Have you seen https://github.com/actions-rs/clippy-check? Would it be preferable to use that instead of actions-rs/cargo with a clippy argument? The lint annotations look nice 😃

(I will create another PR in the near term to fix the last correctness lint we have)

merged

[Enselic]

Have you seen https://github.com/actions-rs/clippy-check?

Yes and it looks very nice 🤩 But I interpret

Limitations
Due to token permissions, this Action WILL NOT be able to post clippy annotations for Pull Requests from the forked repositories.

to mean that it will not work for PRs that contributors creates, which makes it almost useless IMHO 😞 So I don't think it is worth setting up I'm afraid. I would be super great to have if it worked for PRs, for sure.

I'm glad you agree with my proposal! I'll update the PR.