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

Add cargo-semver-checks to linting category #105

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

obi1kenobi
Copy link
Contributor

cargo-semver-checks is used by many large projects in the Rust ecosystem, such as tokio and even cargo itself. Add it as a recommendation in the linting category.

If you feel a different category would be a better fit, I'd be happy to move it there.

Thanks for maintaining this list!

`cargo-semver-checks` is used by many large projects in the Rust ecosystem, such as `tokio` and even `cargo` itself. Add it as a recommendation in the linting category.

If you feel a different category would be a better fit, I'd be happy to move it there.

Thanks for maintaining this list!
@nicoburns nicoburns enabled auto-merge (squash) April 18, 2024 10:35
@nicoburns nicoburns merged commit 2334f85 into nicoburns:main Apr 18, 2024
2 checks passed
@obi1kenobi
Copy link
Contributor Author

Thanks!

@obi1kenobi obi1kenobi deleted the patch-1 branch April 18, 2024 21:52
@epage
Copy link

epage commented Apr 18, 2024

Speaking personally, I think this should be reverted.

I am very excited about cargo-semver-checks and look forward to the potential of what it can do for the ecosystem but there is a fundamental workflow flaw at this time that means I cannot endorse it and I think it should generally not be endorsed.

cargo-semver-checks, and the associate action, encourage people to bump their version in Cargo.toml during development. This breaks a fundamental workflow within Cargo, [patch]. You can only patch a dependency if the version field matches between what you have locked and what you are patching in. For example, if I depend on a project that has adopted the common cargo-semver-checks workflow, I can't patch in the git repo to test if a fix addresses a problem to verify it before it gets released.

@nicoburns
Copy link
Owner

You can only patch a dependency if the version field matches between what you have locked and what you are patching in.

Hmm... that seems non-ideal. What is the reason for this restriction? Could it be relaxed on the cargo side? (perhaps make it a warning rather than a hard error?). Because even aside from cargo-semver-checks this seems like a very common workflow. And if you're patching something then you should be on the lookout for breakage anyway.

@obi1kenobi
Copy link
Contributor Author

obi1kenobi commented Apr 18, 2024

encourage people to bump their version in Cargo.toml during development

@epage, I don't think this is a fair or accurate representation of the facts.

We explicitly encourage people to use the action as part of a cargo publish workflow only at this time. I've been saying so at every opportunity: in talks, in docs, and in blog posts. You've opened two issues on the action's repo mentioning that the action isn't yet suitable for running on PRs, and they both remain open since we haven't fixed them yet.

Of course some users have come up with workarounds that let them run the action on PRs too. Some of them even just straight-up run the action on PRs and make it a non-required step. But I don't think it's fair or accurate to say that we encourage this when I've done everything I possibly can to discourage it.

Even for local workflows, this is still not an accurate representation. I don't see a reason why a maintainer would have to bump their local Cargo.toml version number — it doesn't seem to offer anything that some combination of the --release-type, --baseline-rev, and/or --baseline-version flags won't also do in a less clunky way.

@djc
Copy link
Collaborator

djc commented Apr 19, 2024

IMO there are still other good reasons to bump versions during development and the imposition of that workflow on patch at least in my experience has not been a big deal, so I don't think it's a good reason to drop cargo-semver-checks from this page.

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.

5 participants