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 lints on nonchanged crates #206

Merged
merged 33 commits into from
Dec 21, 2022

Conversation

tonowak
Copy link
Collaborator

@tonowak tonowak commented Dec 3, 2022

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.

@tonowak tonowak requested a review from obi1kenobi December 3, 2022 14:09
@tonowak
Copy link
Collaborator Author

tonowak commented Dec 9, 2022

This git merge produced quite a lot of commits in the Conversation page here. Was there a way to avoid it?

@obi1kenobi
Copy link
Owner

This git merge produced quite a lot of commits in the Conversation page here. Was there a way to avoid it?

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.

@tonowak
Copy link
Collaborator Author

tonowak commented Dec 18, 2022

One possible issue: probably cargo test is now even slower.

src/query.rs Outdated Show resolved Hide resolved
Copy link
Owner

@obi1kenobi obi1kenobi left a 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.

src/query.rs Outdated Show resolved Hide resolved
src/query.rs Outdated Show resolved Hide resolved
src/query.rs Outdated Show resolved Hide resolved
src/query.rs Outdated Show resolved Hide resolved
Copy link
Owner

@obi1kenobi obi1kenobi left a 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.

@obi1kenobi obi1kenobi merged commit 1567ecc into obi1kenobi:main Dec 21, 2022
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.

2 participants