What should the CLI look like?

[epage]

Current, everything is under cargo semver-checks check-release. Deeply nested subcommands like that should be in rare cases and we should be cautious of having long command names like that.

Do we need that extra subcommand? Is there another way of expressing that idea?

Let's first breakdown expected workfows (which will help with #47).

The overall aim is to help with API evolution,. including

• Report to reviewers that a breaking change was made when it shouldn't
• Report to reviewers that a breaking change was made to ensure it gets documented
• Check local changes for unexpected breaking changes
• Check local changes for breaking changes to document
• As a user of a crate, find out what changed in its API
• Get a suggested minimal version to upgrade to
• Check that we aren't upgrading to too small of a version

[obi1kenobi]

Possible additional operations:

• Run user-specified queries to check additional invariants
  • Example courtesy of @rcoh: "ensure third-party crates' types (modulo explicit allow-list) aren't leaking through this crate's API"
• Get a suggested kind of version upgrade (patch / minor / major), as opposed to a specific version number
  • Useful if labelling a GitHub PR with the outcome, since the label would be something like M-semver-minor and not a specific version number.

[obi1kenobi]

To me, at least the "About to publish, please lint all" and the "Checking a PR, advise on semver changes" modes seem different enough to be subcommands rather than flags.

That said, I admit that I have only minimal experience designing CLIs, so it's entirely possible I'm missing something obvious.

[epage]

Example courtesy of @rcoh: "ensure third-party crates' types (modulo explicit allow-list) aren't leaking through this crate's API"

I know this is just an example but Isn't this a lint against a a specific snapshot rather than in comparing snapshots? There is also an RFC for this

[obi1kenobi]

It can be phrased and implemented as "no new third-party types leak through compared to the last version" as well, and I'm sure I can come up with more example custom lints that don't have RFCs or whose RFCs have stalled and might not happen. I think cargo-semver-checks or a tool very much like it can be a good way to address the user need without requiring ecosystem-wide changes first.

In general, though, I'm not sure every lint must always be across snapshots. Some of the lints in #5 are only over the "current" version. I'm not sure how to best make that work in the CLI, but the query engine can run those queries just fine — in adapter.rs the baseline crate info is always optional.

[epage]

In general, though, I'm not sure every lint must always be across snapshots. Some of the lints in #5 are only over the "current" version. I'm not sure how to best make that work in the CLI, but the query engine can run those queries just fine — in adapter.rs the baseline crate info is always optional.

Isn't that what clippy is for?

How do we draw the line for what belongs in clippy vs cargo-semver-check

[obi1kenobi]

I don't think it has to be an either-or. Some lints are easier to write in clippy, some will be easier to write as Trustfall queries, and let users pick what is best for their use case. Overlap in capabilities isn't necessarily bad, in my opinion.

An argument against "just let users write their custom lints through clippy" is that they could have done so already, but haven't because of the complexity and learning curve. Multiple people I spoke to within a few days or RustConf were excited at the idea of writing lints as custom queries because it would lower the barrier to entry and make more lints worth implementing.

If you don't think cargo-semver-checks is the right vehicle for custom lints, then an alternative would be to expose cargo-semver-checks Trustfall adapter as a library and make a separate cargo tool for those queries. I'm open to the idea if you think that's best.

But I do think that "here's one tool you can run to make sure you're ready to publish" is a better user story than "here are N tools you should run before you publish, which under the hood function very similarly." Custom lints are just one more thing that maintainers want to check before publishing. Which gives me a tool name idea: cargo preflight (just reserved it), like a pre-flight checklist.

[epage]

I don't think it has to be an either-or. Some lints are easier to write in clippy, some will be easier to write as Trustfall queries, and let users pick what is best for their use case. Overlap in capabilities isn't necessarily bad, in my opinion.

A couple of angles on this

• This is a symptom that clippy needs to improve and they are talking about it and I brought up trustfall with Manish
• If the aim is for merging into cargo, then we need to be clear on scope.

If you don't think cargo-semver-checks is the right vehicle for custom lints, then an alternative would be to expose cargo-semver-checks Trustfall adapter as a library and make a separate cargo tool for those queries. I'm open to the idea if you think that's best.

I can see creating a stop gap tool for user-driven lints until clippy supports it (if they ever do).

[obi1kenobi]

I have no strong preference one way or the other, but I am curious whether you think custom queries in general should be in scope.

They can easily be called altogether out of scope, so long as cargo-semver-checks is usable as a library and exposes the impl Adapter type through the library API. Then I or someone else could easily build a tool to run arbitrary queries through that adapter over any rustdoc JSON file or combination of them.

[epage]

I have no strong preference one way or the other, but I am curious whether you think custom queries in general should be in scope.

I see value in supporting custom queries in the long term for clippy and semver-checks. Stablizing the format for doing custom queries could take some time though. I can see support for them existing behind an unstable CLI flag.

[ahollmann]

Thanks for this great tool.

It would be great to use it non-published crates and testing features branches for breaking changes against main/master.

Something like buf breaking --against '.git#branch=main' would be great (https://docs.buf.build/breaking/usage).

[epage]

It would be great to use it non-published crates and testing features branches for breaking changes against main/master.

This was being tracked in #51 and was implemented in #87. You can pass the --baseline-rev main flag.

Now, for the action to support it, that is different. If you have a need for more configuration there, I'd recommend creating an issue against the action.

[obi1kenobi]

After speaking to users, I've changed my mind on this. I think we'd be better off deprecating (and eventually, in the merge into cargo, removing) the semver-checks check-release subcommand, and moving everything to the top level semver-checks command.

I'd propose making check-release optional, and accepting cargo semver-checks as if it were cargo semver-checks check-release.

Here's the current CLI + some possible additions, in part inspired by cargo test and cargo clippy:

cargo semver-checks [check-release]
    --workspace
    --exclude <CRATE>
    --manifest-path
    -p, --package <SPEC>
    -v, --verbose (repeatable 2x)
    -q, --quiet
    --bugreport
    -V, --version
    --current-rustdoc <JSON_PATH>
    --baseline-rustdoc <JSON_PATH>
    --baseline-version <X.Y.Z>
    --baseline-rev <REV>
    --baseline-root <MANIFEST_ROOT>

future flags to consider:
    --features/--all-features     (current default is --all-features)
    --explain <EXPLAIN>           (explain a lint, like `cargo clippy`)
    --keep-going                  (unstable in cargo, should we add it?)
    --baseline-cache <CACHE_DIR>  (registry-based baselines don't change and are expensive to regenerate)

[tonowak]

The --manifest-path and --baseline-root have too different names for me and I never can remember what is the name of the --baseline-root command. I understand that --manifest-path tries to be similar to cargo test etc, but to avoid confusion, I think it's better to name it --current-manifest-path. Then, renaming --baseline-root to --baseline-manifest-path would be closer to the current style.

Similarly, I think a bigger distinction in the names of rustdoc generation would be a good change. For example, it's not clear when to pass --baseline-version based only on the list of CLI flags. In fact, there are three/four types of generation: from registry, git revision, project path and rustdoc path. When one knows what are the types it's easy to understand which flag does what, but I don't think the users would know about them and that they would know that it's not possible to pass two flags of different rustdoc generations.

Also, I'm currently thinking about making a PR to add rustdoc generation of current version from the registry, similar to the baseline one. It would allow to easily use the tool for data gathering of previous releases (and would shorten the code of #207).

Going with this idea, why don't we support the same types of rustdoc generations in current as in the baseline? That would align with the idea of prefixing the commands with current- and baseline-.

[obi1kenobi]

Whenever a flag is doing something that cargo itself does as well, it should have the same name as the corresponding flag in cargo to make the merge easier (#61). Otherwise, we'd be forcing users to learn different flags for different cargo commands, which isn't a good idea.

--baseline-root doesn't specify a manifest, but a directory containing the manifest and corresponding source code, so handling that as a rename would change the semantics as well. It's not clear to me whether we'd want --baseline-manifest-path instead of, or in addition to, --baseline-root. @epage do you have any opinions?

Keep in mind that we shouldn't just flat-out remove and rename any flag, because that would be quite annoying to our users. A better process would be to deprecate the flag, add the new flag, and allow users a reasonable amount of time to switch their scripts and workflows to the new flag.

When one knows what are the types it's easy to understand which flag does what, but I don't think the users would know about them and that they would know that it's not possible to pass two flags of different rustdoc generations.

This is fair. Can we help users understand the "current vs baseline" concept somehow? For example, with a better README, or with better --help text?

Going with this idea, why don't we support the same types of rustdoc generations in current as in the baseline?

That's fine by me. Just keep in mind that --current-root doesn't make sense since that's handled by --manifest-path already. --current-version and --current-rev should be fine to add.

Another option, if you're interested in adding it, is to allow combinations of --baseline-root + --baseline-rev, in case we need to first check out a git revision, and then navigate to a path that isn't where we are now. The difficulty in this task is writing excellent documentation describing the behavior, rather than the implementation of the code itself.

We also don't have any test cases for most of these flags, which would be another great thing to add, perhaps in a style similar to our existing GitHub Actions tests on real-world codebases.

[tonowak]

Keep in mind that we shouldn't just flat-out remove and rename any flag, because that would be quite annoying to our users. A better process would be to deprecate the flag, add the new flag, and allow users a reasonable amount of time to switch their scripts and workflows to the new flag.

The renaming can be done during the removal of semver-checks check-release subcommand. The flags under check-release and under directly semver-checks don't have to be the same -- the users still need to alter their existing commands.

[obi1kenobi]

I still feel it would be confusing to have some but not all flags mirrored between semver-checks and semver-checks check-release. It feels like we're just setting up our users for frustration without getting any huge benefit.

I'd rather save the hard renames for the actual merge into cargo, at which point the tool itself might get renamed too (#47).

[epage]

After speaking to users, I've changed my mind on this. I think we'd be better off deprecating (and eventually, in the merge into cargo, removing) the semver-checks check-release subcommand, and moving everything to the top level semver-checks command.

atm all that is supported is "report what isn't compatible with the current version vs the base version", right?

How do we plan to handle the other use cases? I believe resolving that was the main blocker for settling on the CLI.

[obi1kenobi]

atm all that is supported is "report what isn't compatible with the current version vs the base version", right?

That's right.

How do we plan to handle the other use cases? I believe resolving that was the main blocker for settling on the CLI.###

I think an approach similar to cargo-edit may be preferable: separate binary targets in the same package where it makes sense, and flags on existing commands where it makes sense.

A rough draft example:

• the changelog mode could be its own command in its own binary
• a command to generate rustdoc for a registry crate (i.e. not one in a local repo) based on its name and version would be useful (it would enable @tonowak's work of scanning crates.io for existing semver violations, for example)
• use flags on the existing semver-checks command for the PR-centric workflow, where the Cargo.toml version might not be updated yet and we're looking to determine what minimum version bump would be necessary to satisfy semver

I think this is better since each of the commands would be simpler to understand, and would simplify the merge into cargo since it would be more in line with cargo's current approach anyway.

It would also avoid the issue of users running cargo semver-checks --help (instead of cargo semver-checks check-release --help), and encountering help text that doesn't describe the various ways to generate a baseline etc. This is something I saw happen several times in user tests, and what ultimately convinced me that my original approach wasn't the right one.

What do you think?

[epage]

If this is a "experiment and see", then I'm fine. If its "the answer" and closes this PR, then I'm not.

I would want to see at least prototypes of the above for us to see how it works in practice and to experiment with alternatives.

[obi1kenobi]

Here's what I'd propose for right now:

• Add a new feature, unstable, and gate the new binaries behind it so we can experiment without committing to an approach.
• Adjust the CLI so that if no subcommand (like check-release) is supplied, then check-release is implicit. This solves the --help confusion and I believe would match the mental model that new users seem to start with.
• Leave this issue open, and re-evaluate in a few months after we've had a chance to see how the above plays out in practice.

What do you think?

[epage]

Sounds good.