-
-
Notifications
You must be signed in to change notification settings - Fork 79
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 lints on nonchanged crates #206
Run lints on nonchanged crates #206
Conversation
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
This |
Not that I know of. Also there are now a lot of merge conflicts. The workflow of having PRs that depend on other unmerged PRs is called "stacked commits" and it's worth reading about, if you're interested in learning how to perfect it. |
One possible issue: probably |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR can still benefit from comments in the code. Yes, the assert_no_false_positives_in_nonchanged_crate
function name is pretty good, but adding a comment that goes one step further and explains why anything found in an unchanged crate is a false-positive (and why we're even bothering to test this) won't hurt anyone. For example, it would be perfectly understandable if a new person reading the code assumed that testing a crate against itself is guaranteed by construction to return no lints — that getting lints in that situation would have caused a type error first. They have no way of knowing this is not true, and also no way of knowing that this sort of check has failed on real-world crates in the past because of obscure reasons. I certainly could not have imagined the ways in which this invariant failed to hold in the real world.
Here's the high-level view: we want cargo-semver-checks
to be a successful project with a long life and a community of maintainers. People have limited time and energy to contribute, and many projects they could be contributing to. Why would they contribute to a project whose code is confusing, if they could contribute to a project that is easy to understand and has a welcoming codebase with code comments explaining possible sticky situations?
Thus far, the emphasis was on making cargo-semver-checks
minimally viable as a tool: useful functionality that solves a real-world problem, users that like it, etc. We've succeeded, and it's already more than minimally viable: it runs in the repos of many established projects, and has prevented real-life semver violations. We now have to also start thinking about clean code that contributors can dive into, that lets us build and ship new features quickly and without bugs, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. As soon as you get a chance to sort out the last couple of clippy lints, I'm happy to merge.
This PR is a follow-up to #203. In the tests, it runs each lint on all testing crates, comparing the difference of a crate with the same crate (and it expects that there is no difference). Thus, it is an additional check to be sure that there are no false-positives.
I'm not sure how GitHub behaves when I've created a PR that is built on another, not yet merged PR. I hope that what I did makes sense, but if not, I can close the PR and reopen it after the previous PR will get merged.