Feedback on `cargo-add` before its merged

What is the deprecation plan for external cargo add? Will the merged subcommand override existing cargo-add binary, or the other way around? If a user currently does cargo add --help || cargo install cargo-add in some devtooling script (Makefile, custom subcommand), how will it behave and what will end up at cargo add?

Is the plan to update (and keep updating) external cargo-add such that it generally exhibits the same behavior as the merged one? I think that would be the sanest way to ensure that we don't end up with a situation similar to how shellscripts rely on bash-isms (assuming merged cargo add will have a subset of features currently available), i.e. "external cargo add isms"

1 Like

Will the merged subcommand override existing cargo-add binary, or the other way around?

Built-in cargo commands supersede external ones.

Is the plan to update (and keep updating) external cargo-add such that it generally exhibits the same behavior as the merged one? I think that would be the sanest way to ensure that we don't end up with a situation similar to how shellscripts rely on bash-isms (assuming merged cargo add will have a subset of features currently available), i.e. "external cargo add isms"

I am updating cargo-edit to match the proposal for what will be merged into cargo. However, to merge it into cargo, I'm effectively going to have to rewrite cargo-add. This is safe because of our extensive set of end-to-end tests (if you haven't used the trycmd crate, I highly recommend it). Because the code bases will have diverge so much, it seems unlikely they will be kept in sync.

1 Like

ok, i propose to emit a warning if cargo-add binary is found while executing the builtin cargo add, and recommend deletion of that binary in the message. seems like the easiest way to prevent confusion

3 Likes

This is now fixed for cargo-add.

You are right, we are not allowing people to use cargo add to have multiple versions of the same crate with --rename. This is now fixed in master.

1 Like

As a quick update, all of the "add" workflow changes are in master, leaving just the "modify" workflow.

You can check out the -h output or install it via cargo install --git https://github.com/killercup/cargo-edit cargo-edit

Summary of UI changes:

8 Likes

Do we really want to move to that syntax in the command-line tooling, considering the prior art of @ syntax being extremely popular in other languages? Are there many other languages where the CLI tooling for adding dependencies uses the : syntax? Even if the version separator would be : in the configuration files that get generated, or some machine-generated & machine-parsed output of other tools, I want to voice a bit of a concern for unfamiliarity / strangeness budget when it comes to the commonly-used user-facing interfaces.

Anecdotal edit: I use cargo add i.e. cargo-edit pretty much every single time I write Rust. Same with yarn add or npm install whenever I write JavaScript. Conversely, I had not even heard of cargo-pkid before. The "common-ness" factor of usecases & prior art in other languages should probably be taken into account here. Users are probably way more likely to expect @-syntax to work, even if cargo-pkgid currently uses : syntax.

6 Likes

Presumably, if any shorthand version syntax (the @ or : or whatever) gets added to mainline cargo add, the same syntax will also be added to mainline cargo install?

I agree; I think @ has precedent in many tools to specify a version. "This package, at this version" (in the same sense as "at this point in time"). And for that matter it's what people are used to cargo-add supporting out-of-tree. It doesn't seem worth trying to change that.

I'd love to see us supporting @ elsewhere in Cargo for consistency.

6 Likes

After some discussion with the cargo team, I've reverted back to using @. We'll evaluate (1) switching cargo to use @ in addition to : and (2) adding support for @ in other commands.

12 Likes

With how productive this thread has been, I want to run an idea from a recent issue by you all.

I am wrapping up work for re-running cargo-add being additive. This is getting us closer to filling the role of cargo-feature. What if closed some of the gap with cargo-feature and added inline feature activations?

So in addition to

$ cargo add serde --features derive
$ cargo add serde_json

You could do

$ cargo add serde +derive serde_json

Where any positional argument starting with a + is a feature for the preceding crate.

Benefits

  • Shorter way of adding features (we've had requests for a -f flag but we've wanted to match cargo's shorts/longs)
  • One less flag that only works in "single crate" mode (as opposed to "multiple crate" mode)
    • This both means fewer commands to run but also means it becomes less important to explain to the user the modality of cargo-add (and even as a VIM user, I understand the challenge people have with modality)
  • With a single-command to get up and going, I can see it being more likely for this to be used in documentation
  • If you squint real hard, this sort of fills part of the role of @Kixunil's "groups" feature request (#383).

Downsides

  • There is more syntax in a single argument, making it more difficult to explain how to use it
  • Slippery slope of wanting to cover every cargo feature use case (really, the only thing left is feature removal but that might be weird in an add command)

See also #592.

3 Likes

I personally do write quite a lot things like these:

$ cargo add tokio --features macros --features rt-multi-thread --features sync

I don't know if a shortcut for this already exists but I would very much appreciate the one you mentioned.

I'd just say if you do add +derive please let + syntax work after --features i'd say there is at least similar precedent in the compiler with rustc -C target-feature=+whatever, I think it allows feature removal too via '-whatever'.

The thing I would note is that the inverse feature removal option -derive looks like a weird option to cargo add rather than specifying whether a feature should be added or removed, while the syntax of target-feature conspires to make it look less option like.

I like the concept of +feature as a shorthand, though I'd be hesitant to make last-minute changes like that before adding it. cargo-add in its current state has had a long time to bake in public; if we add anything new we may want to stick it behind -Z unstable-options at least for a little while.

11 Likes

I have on multiple occasions tried to provide a comma-separated list (like cargo add tokio --features macro,rt-multi-thread,sync) and have always been disappointed when it didn't work. Having some sort of shorthand would be wonderful, but I agree with @josh that it should be done unstably at first.

3 Likes

@tvallotton, @jhpratt: you can use a space separated list. That’s actually a good point though, iirc a comma is an invalid character in a feature so a comma separated list could also be supported.

2 Likes

I personally feel like the need to add lengthy extras like “-Z unstable-options” as well as the need to use nightly is a significant hindrance to properly trying out features of a tool that’s mainly about convenience and making additions to Cargo.toml as low-overhead as possible.

I’m not sure I understand where the urgency of merging cargo-add into cargo comes from that I perceive here. For now, using cargo-add (as part of cargo-edit) instead of anything built-in doesn’t seem like a particularly bad situation at all, IMO. I feel like this thread has come up with some relevant issues as well as great ideas on how cargo-add can be further improved, and it would – in my mind – make a lot of sense to let the takeaway of this be that cargo-add was not as complete as we might have originally thought, and hence perhaps shouldn’t be merged with cargo yet? Maybe it makes more sence once all the improvements / changes discussed here are a) implemented and b) had some additional amount of time to “bake” further. Give it a few months while communicating the sentiment of “we want to merge this into cargo soon-ish”, and we can probably get enough feedback rather quickly to be certain that cargo-add is in a good/working/stable state without any (significant) issues.

If merging soon is really necessary / strongly wanted for some reason, then stabilizing a minimal known-good subset of available options seems like the only thing that can be safely done. However, as for not killing user experience for existing users, I believe there would still be a need to keep exposing a fully-featured unstable version of the command under a different name in the cargo-edit crate. Maybe something like cargo addu (the “u” stands for “unstable”). This way it’s also possible for unstable features to be “actually” properly tested without the need for significant overhead in the length of the command. Also, for proper user experience, I would expect that an integrated, yet not yet feature complete cargo add should recognize when users try to use unstable options, and – especially if they have cargo-edit installed – recommend a) to update their cargo-edit if it’s still at a version without cargo-addu, and b) recommend using cargo addu instead to get access to those still-unstable/missing options.


Speaking for myself, I would e.g. love to see cargo add become completely additive and replace all additive use-cases of cargo-feature, but I also know that I would personally never actually try out this capability in practice if it required something like cargo +nightly add -Z unstable-options crate-name +feature-name. That’s just way too long, I would always use cargo feature crate-name +feature-name instead; only if it was as short as e.g. the cargo addu crate-name +feature-name I talked about above (with cargo-edit installed, which I have installed anyways), I would probably make the switch from using cargo-feature for this, and thus be able to recognize and report any potential bug or issues.

10 Likes

I completely agree that -Z unstable-options is not ideal for a quick and convenient command. I wouldn't expect anything added behind that to remain so for very long; I wasn't necessarily suggesting it for all future additions, just for things added right before merging so that they didn't become immediately stable with minimal time or testing.

My concern is that we don't want to let the perfect be the enemy of the good here: if we don't want to add cargo-add until it's substantially further improved, we won't add it for a long time. And while some folks will happily use cargo-add while out-of-tree, I'm expecting that many folks will find it more convenient or even discover it for the first time because it's in tree. We can also start doing things like providing cargo-add instructions on crates.io, or otherwise assuming its presence.

1 Like

Have we considered adding a way for a cargo extension to override a cargo subcommand?

While it makes sense to prioritize subcommands by default, in our case the problem is that we want cargo add to be included by default, but we also want the benefits of an extension like cargo-edit which can change its API on a whim.

For me, the obvious solution would be to have the default cargo add, and also the "overloaded" cargo add provided by cargo-edit with additional options. That way, no need to bother adding the -Z unstable-options argument.

1 Like

I had assumed cargo-add was using clap for the splitting which only supported one character, so I was waiting on merging cargo-add into cargo to use its facilities for feature flags.

I was wrong so I quickly added support for , for separating features: fix(add): Allow ',' to separate features by epage · Pull Request #599 · killercup/cargo-edit · GitHub

1 Like