Skip to content

Enforce clippy in CI #406

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

Merged
merged 3 commits into from
Sep 28, 2022
Merged

Enforce clippy in CI #406

merged 3 commits into from
Sep 28, 2022

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Sep 26, 2022

Depends on #394

CI wasn't actually enforcing clippy, because actions-rs/clippy-check needs to run as pull_request_target to get the token to add the "fancy" comments to the diff. It does nothing when ran as pull_request.

pull_request_target is troublesome because it uses the yaml from master instead of from the PR branch, which means when bumping clippy you don't know whether CI passes until after it's merged. I've made it use cargo clippy instead, which works on pull_request.

@Dirbaio Dirbaio requested a review from a team as a code owner September 26, 2022 11:27
@rust-highfive
Copy link

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

I am not a fan of enforcing clippy. Using the nightly clippy is probably even more volatile.
Granted, it is pinned to a version that works fine now, but we will update it regularly and I see us making unnecessary/unrelated/even contestable changes in unrelated crates in order to land PRs.
For me, the failing ❌ icon in CI is enough warning when reviewing a PR.

@@ -259,7 +259,7 @@ impl AddressMode for TenBitAddress {}
/// Transactional I2C operation.
///
/// Several operations can be combined as part of a transaction.
#[derive(Debug, PartialEq)]
#[derive(Debug, PartialEq, Eq)]
Copy link
Member

Choose a reason for hiding this comment

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

This should be noted in the changelog.

@Dirbaio
Copy link
Member Author

Dirbaio commented Sep 26, 2022

If we keep clippy but not enforcing it, the moment we decide to not fix a warning because we don't agree with it, all following PRs will have the red ❌ forever, due to an issue unrelated to the PR. At that point it stops being a useful flag for code review, and it's also confusing for contributors because a red ❌ in a PR usually means you did something wrong.

IMO if we keep it we should enforce it, otherwise we should remove it.

@Dirbaio
Copy link
Member Author

Dirbaio commented Sep 26, 2022

making unnecessary/unrelated/even contestable changes in unrelated crates in order to land PRs.

Random warnings won't appear when landing a feature PR, because the clippy version is pinned. We can update it in dedicated PRs, so the changes for any new warnings don't mix with normal PRs.

@eldruin
Copy link
Member

eldruin commented Sep 28, 2022

Up to you @ryankurte, @therealprof

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

bors r+

@bors bors bot merged commit e77a90b into master Sep 28, 2022
@bors bors bot deleted the enforce-clippy branch September 28, 2022 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants