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

Run clippy on a fixed version of rustc #18

Open
epage opened this issue Jul 13, 2019 · 3 comments · Fixed by #29
Open

Run clippy on a fixed version of rustc #18

epage opened this issue Jul 13, 2019 · 3 comments · Fixed by #29
Labels
bug Something isn't working

Comments

@epage
Copy link
Contributor

epage commented Jul 13, 2019

By running clippy on a non-fixed version of rustc, CI will break as new clippy's introduce new checks. Having the CI fail on a new contributor can be discouraging and then once they realize it is a sporadic failure, it can give them the sense of the project being not well maintained.

To resolve this, I've found pinning the version of rustc to be the best approach.

@epage epage added the bug Something isn't working label Jul 13, 2019
@jonhoo
Copy link
Collaborator

jonhoo commented Jul 15, 2019

I'm of two minds on this. On the one hand, I agree that it's unfortunate for newcomers to be met with clippy errors on code they didn't introduce, but at the same time I think it's important to keep clippy up-to-date, otherwise it is so easy to miss new true error cases that clippy identifies. Pinning to an older version means that we're losing out on improvements (and, yes, new warnings) from clippy, and authors now have to remember to bump their clippy version (which, my guess is, no-one will remember).

I think my vote here would still be to use clippy from stable, and then have clippy from beta with allow_fail. That way developers will generally be warned in advance if something will start failing, and will hopefully fix it before contributors run into it.

@epage
Copy link
Contributor Author

epage commented Jul 15, 2019

I strongly disagree. One of my guiding principles is that the CI should never be red, especially for new contributors.

Also, I suspect I put a lower value on new clippy warnings. I'm fine with the suggestion of tying clippy to minrust. It is rare for minrust to be more than a couple versions (I think I've seen one case of 15 releases back and one case that goes to rustc 1.0) and I've generally not seen new lints introduced that make me want to upgrade.

@jonhoo
Copy link
Collaborator

jonhoo commented Jul 15, 2019

I think this gets pretty difficult to enforce, especially if we make minrust optional (#20). I generally keep minrust as low as I possibly can for my libraries, which would leave my clippy far behind. In extreme cases it might also lead to a situation where clippy locally tells me to change something, but if I do, clippy on CI will start complaining, which seems like an issue.

I think clippy from stable and allow_fail clippy from beta gives us a decent trade-off. New contributors won't generally see failing CI due to clippy (because the codebase is presumably clean on clippy stable), and developers will be warned early if upcoming clippy releases will break things for them.

jonhoo added a commit that referenced this issue Jul 15, 2019
Too many spurious failures, which lead to people ignoring errors.

Fixes #18
jonhoo added a commit that referenced this issue Jul 15, 2019
Too many spurious failures, which lead to people ignoring errors.

Progress towards #18.
@jonhoo jonhoo reopened this Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants