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

actions: Run optional clippy (linting) checks with the beta toolchain #111

Merged

Conversation

primeos-work
Copy link
Member

@primeos-work primeos-work commented Dec 14, 2022

More recent toolchain versions come with additional clippy lints.
E.g., the current stable version is 1.65.0 and shows three additional
clippy warnings (compared to 1.64.0 / the MSRV). We already saw those
warnings locally (until aa10870).

We only make the check with the MSRV required, as contributors shouldn't
have to fix clippy warnings in other parts of the code (unrelated to
their PR; we'll fix any additional warnings when bumping the MSRV).

However, it is useful to also run the clippy check with a more recent
toolchain version so that we maintainers can noticed and fix new
warnings in advance (without having to run clippy locally).
This is implemented so that one sees the failing optional job (hopefully
that won't confuse contributors) but the "CI" job that is required for
bors will still pass.
Most contributors will likely use the stable version so using the beta
version for the check seems ideal to catch warnings in advance (so that
contributors don't see them when they run clippy locally).

Relevant GitHub actions documentation:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error

Signed-off-by: Michael Weiss michael.weiss@atos.net


@matthiasbeyer what do you think about this? At first I thought it would be a good idea but now I'm not so sure anymore... :o A better alternative (if it can be implemented) might be to use ${{ matrix.rust }} here as well and only enforce the check against the MSRV (since contributors shouldn't have to fix linting issues unrelated to their changes and those fixes should be in a dedicated commit).


With this change: https://github.com/primeos-work/butido/actions/runs/3695146494/jobs/6257271973
Without this change: https://github.com/science-computing/butido/actions/runs/3548987433/jobs/5960839002

@@ -86,10 +86,10 @@ jobs:
- uses: actions/checkout@v3.1.0
- uses: actions-rs/toolchain@v1
with:
toolchain: 1.64.0
toolchain: stable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better alternative (if it can be implemented) might be to use ${{ matrix.rust }} here as well and only enforce the check against the MSRV (since contributors shouldn't have to fix linting issues unrelated to their changes and those fixes should be in a dedicated commit).

Yes this is a better way.

Using toolchain: stable would update the used toolchain as soon as a new version is published (e.g. tomorrow 1.66.0 will be stable, in six more weeks it will be 1.67.0). That would mean that new lints are automatically "there" at some point and contributors would need to fix code that was working before and might not even be related to their contribution.

Thus having a fixed version is always better IMO.

@primeos-work primeos-work marked this pull request as draft December 14, 2022 15:46
More recent toolchain versions come with additional clippy lints.
E.g., the current stable version is 1.65.0 and shows three additional
clippy warnings (compared to 1.64.0 / the MSRV). We already saw those
warnings locally (until aa10870).

We only make the check with the MSRV required, as contributors shouldn't
have to fix clippy warnings in other parts of the code (unrelated to
their PR; we'll fix any additional warnings when bumping the MSRV).

However, it is useful to also run the clippy check with a more recent
toolchain version so that we maintainers can noticed and fix new
warnings in advance (without having to run clippy locally).
This is implemented so that one sees the failing optional job (hopefully
that won't confuse contributors) but the "CI" job that is required for
bors will still pass.
Most contributors will likely use the stable version so using the beta
version for the check seems ideal to catch warnings in advance (so that
contributors don't see them when they run clippy locally).

Relevant GitHub actions documentation:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error

Signed-off-by: Michael Weiss <michael.weiss@atos.net>
@primeos-work primeos-work changed the title actions: Update the toolchain version for clippy (linting) actions: Run optional clippy (linting) checks with the beta toolchain Dec 22, 2022
@primeos-work primeos-work marked this pull request as ready for review December 22, 2022 16:01
@primeos-work
Copy link
Member Author

primeos-work commented Dec 22, 2022

Oh, that is not ideal (need to check if long Markdown hyperlinks are allowed):

Commit cb5dd8ed8a:
23: B1 Line exceeds max length (113>80): "https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error"
Error: Process completed with exit code 1.

@primeos-work
Copy link
Member Author

I'll fix the gitlint warnings in a separate PR, likely together with bumping the edition from 2018 to 2021 (I can also do it in this PR but I'd like to double check if bors merge works as it should (the "CI" check is fine).

I'll also add an exception for hyperlinks in the (to be added) gitlint configuration.

@matthiasbeyer
Copy link
Contributor

gitlint can ignore lines by regex, I think. This should be the perfect case for this 😆

Updating edition should be a seperate PR IMO. But you're the maintainer, not me 😆

@primeos-work
Copy link
Member Author

Ok, let's give this a try then :)

bors merge

@bors
Copy link
Contributor

bors bot commented Dec 22, 2022

Build succeeded:

@bors bors bot merged commit f86b90b into science-computing:master Dec 22, 2022
@primeos-work primeos-work mentioned this pull request Jan 3, 2023
11 tasks
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

Successfully merging this pull request may close these issues.

None yet

2 participants