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

Update smoke test to verify all Clippy and rustc lints #374

Merged
merged 1 commit into from Jul 31, 2023

Conversation

MitMaro
Copy link
Contributor

@MitMaro MitMaro commented Jul 22, 2023

Why

Different projects will have different combinations of rustc and Clippy lints enabled, including lints from the clippy::restriction and clippy::pedantic groups, that are allowed by default. This results in a situation where users of bitflags, depending on their configuration, will have linting errors that they are unable to easily resolve.

What

This updates the smoke test to include all lints from Clippy rustc, that are allowed by default. This allows CI to verify that any changes to bitflags that would result in a failing lint can be caught immediately.

The Clippy lints can be enabled in bulk, since all the lints are contained within one of a small set of rather stable groups. The rustc lints, however, are more difficult, as there are several lints that are not included in any group. As far as I am aware, there is not a way to enable all lint using something like deny(allows), or similar.

Justification

I started making this change because I have clippy::same_name_method enabled in a project, which was causing bitflags to fail. Once I had addressed that particular issue in a local copy, I had issues with clippy::indexing_slicing failing in a different project.

From there I decided to test is there were any further errors, and this was the end result. I decided to not include clippy::cargo as they should never be a problem, and clippy::nursery as those rules often have many false positives.

There are two drawbacks to this approach. The first is that the "list" of lints (particularly from rustc) will need to be maintained as new lints are added. I do not think this will be too much of a burden when compared to the effort of addressing linting issues from users, and not having the lints listed is the current state.

The second drawback, is that as new lints are added to the groups denied for Clippy or rustc, new changes may require additional allowed lints. However, not addressing these lints could result in breaking projects that use bitflags.

@MitMaro
Copy link
Contributor Author

MitMaro commented Jul 22, 2023

I thought about creating an issue before posting this pull request to see if this is something that the project would be interested in. However, I think it's easier to show with a PR.

Please consider this more of a proposal, and if the decision is to not accept this, I will gladly put up a follow-up PR to just address the two lint failures that I found. :)

@MitMaro MitMaro force-pushed the mitmaro/allow-all-linting branch 2 times, most recently from d79268c to 33f4829 Compare July 22, 2023 13:15
Update the smoke test to include all lints and address any issues
discovered.
Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @MitMaro!

It's unfortunate to have to maintain a laundry list of lints to allow/exclude, but at least this gives people a place to drop new lints we need to be aware of as they appear.

@KodrAus KodrAus merged commit 25f5adb into bitflags:main Jul 31, 2023
8 checks passed
@MitMaro MitMaro deleted the mitmaro/allow-all-linting branch August 1, 2023 10:21
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