Feedback on `cargo-add` before its merged

Totally get that.

I was hoping we could recommend aliases for working around the need for -Z but processing of aliases happens after + toolchain selection, so we can't. The next best horrible option is to do it in your shell.

I don't feel a big hurry because, honestly, I don't use cargo-edit. My work on this started more from the perspective of finding areas of high-demand but unfinished cargo work and help it get over the edge. However, the reason I don't use cargo-edit is an anecdote in favor of getting it sooner than later: its because its not been built-in and its one of those quality of life improvements that the effort to install just never seemed worth it but having it integrated, it would be worth it. In effect, I'm doing this work so I will start to use cargo-edit.

I can see the value for waiting. As a parallel, it always feels weird when I see a crate go "1.0" and do major breaking changes. In our case, a lot of this is fairly solid feedback with existing precedence and anything controversial is being put behind a gate or removed. I also need to rewrite cargo-add to fit within cargo and the gap between solidifying the UX and being merged will help. I intentionally started with the UX improvements before porting so (1) we can get feedback and (2) the rewrite will just be implementation details.

It is also unclear how much feedback we'd get from waiting. cargo-add hasn't been getting much feedback in the time I've been involved. The lack of feedback could in part be from the lack of involvement from maintainers before I got involved.

This could be worthwhile but it'd be work. I'm going to be forking the cargo-add code base to merge it into cargo. We'll be having to keep two distinct code bases in sync with each other.

I'm not a fan of +feature. It's a unique microsyntax, close to rustup's +toolchain (cargo +nightly add != cargo add +nightly), and I don't think it will be discoverable.

Cargo already has --features, and I prefer consistency over saving keystrokes. I use cargo test --features=foo,bar often, so cargo add --features=foo,bar seems natural to me. I presume cargo test +foo +bar isn't going to work, so the +feature would be just for this one command.

The + syntax isn't self-explanatory, so in documentation where I recommend users add my crate with features, I'll continue use the explicit version.


I would like to have some syntax to mean "use default features, except the ones specified" (e.g. disable the std feature). This will need a new feature syntax in Cargo. Something like --features=-std could work, but then it would not work well with bare -std or +-std. So I think it's best to leave out clever new feature syntax for now to leave room for future expansion.

10 Likes

Keep in mind it won't be cargo add +nightly but cargo add foo +nightly (for the rare times crates have a "nightly").

While this has the side benefit of saving keystrokes (which is a big benefit, people have been eager for a feature shorthand), the motivating benefits were

  1. Single-line "getting started"

Being able to post this in docs:

$ cargo add serde +derive serde_json

While I know there is the concern about understandability for users, it is much more likely for someone to offer a single-line for getting started than having to document

$ cargo add serde --features derive
$ cargo add serde_json
  1. Reduction in modality.

Our options are

  • Keep modality front and center
  • Remove multi-add support to remove modality
  • Provide + to reduce the need to understand modality (proposed)
  • Provide + and completely remove single-add mode

We should be weighing impact of understanding modality vs understandability of the + syntax.

While +derive is more understandable in a command for features, I think a simple example in --help will clarify the intent in the code because people are used to a derive feature. The main impact will be people completely new to Rust and people starting from the docs and trying to understand the docs vs a blind template. This doesn't mean there is no impact; I'm just trying to frame the impact.

2 Likes

Honestly, I think the 2-line version is way better. The crate names used form a column visually, so it's easy to see at a glance what these commands do. OTOH with the microsyntax feature names mixed with crate names make both unclear. If there were more crates I'd prefer multi-line list than a one wrapping line.

I also find it odd that the syntax is serde +derive ("serde" "+derive") in two arguments, rather than one argument per dependency. I'd expected it to be like Cargo's existing package spec syntax, which is one string with its own syntax, rather than multiple command arguments. Does serde @1 work the same way? I presume not, and serde@1+derive is ambiguous. So it's an odd multi-argument mix where space before version is forbidden, but space before a feature is required.

2 Likes

Hmm… additional effort for keeping diverging code bases in sync seems maybe a bit pointless. I’m not sure about all the technicalities here, could perhaps a cargo-addu tool be created simply as a simple wrapper around a call to cargo +nightly add with unstable options activated? (It may as well additionally print a warning message that it can change or break in the future due to relying on unstable cargo features.) Sure cargo(stable)-calls-“cargo-addu”-calls-cargo(nightly) is a bit messy, and one would need to make sure to provide reasonable error messages e.g. when no nightly toolchain is installed; but it seems better than the “you’ll need to write your own shell script” approach.

Alright, I think the overwrite mode is starting to shape up with

That just leaves unstable inline feature addition and I'll start banging away making this friendly to the cargo code base.

For anyone up for it, please switch to master and report any feedback you have!

2 Likes

Not that it matters publicly, but you can provide an array of chars to str::split. Thank you, though! I very much look forward to having this work in the manner I expect.

1 Like

And we now have unstable, inline feature activation.

Definitely something worth exploring.

I'm with Kornel on the +feature syntax. I'd prefer sticking with --features.

I can understand that. It'd would help if this feedback is verified by trying out in practice. Sometimes giving an honest attempt changes our minds, sometimes it doesn't.

I am keeping this feedback in mind though I also know we can't satisfy everyone and we'll need to weigh out the feedback from a variety of people with overall design goals.

That said, there is another route we can explore depending on the reception

  • Add a short flag for feature (and have approval of that act as as a proxy for approving it for all of cargo)
  • Make the various crate flags position-sensitive. Clap tells us the relative position of each occurrence of a flag so we can map flags to crate names.
  • e.g. cargo add serde -F derive serde_json would be like cargo add serde +derive serde_json

We would have a more explicit and more complete version of + arguments and completely remove modality. It would come at the cost of increased implementation complexity and any potential user confusion due to the rarity of position-sensitive flags.

Adding -F as a shorthand for --features seems safe.

3 Likes

I like the -F flag. My only concern is that it should be equivalent to --features:

cargo add serde --features derive serde_json
# should be equivalent to
cargo add serde -F derive serde_json
# and NOT
cargo add serde -F derive -F serde_json

My proposal is that both -F and --features accept a single feature, or a comma-separated list of features:

# the following are equivalent
cargo add serde -F derive -F unstable serde_json
cargo add serde -Fderive -Funstable   serde_json
cargo add serde -F derive,unstable    serde_json
cargo add serde -F=derive,unstable    serde_json
cargo add serde --features derive --features unstable serde_json
cargo add serde --features derive,unstable            serde_json
# etc.

Note that clap automatically parses -xfoo and -x=foo the same way as -x foo.

4 Likes

-F is in and its just a short flag for --features. Both accept space separated (requires quotes) or comma separated list of features like other cargo commands.

I have not implemented position-sensitivity yet, waiting to see how the reception is, especially from those who are concerned about +.

4 Likes

I think -F derive serde_json is a better idea than +derive serde_json syntax. Thank you for your hard on making cargo-add better. Edit: updated for clarity.

I just want to make sure we are on the same page. Are you saying just -F is better or are you also referring to the proposal for position-sensitive arguments?

Why not make a strict separation of what's an API and what's a strict UI?

We already have an API: Cargo.toml. Let cargo-add break!

1 Like

Speaking about the cargo add serde +derive syntax: looks like only pip has prior art for that (of the add commands listed in the initial post) and uses package[some,extras] for that. Maybe something like that should be considered

# Positive
cargo add serde[derive] serde_json
# Can be extended to negative too
cargo add nom[-std,alloc]
2 Likes

One thing that came up recently: if a crate depends on A which depends on older, semver-incompatible version of B then cargo add B should use the version that's already in the crate tree for two reasons: de-duplication of code and avoiding compatibility problems. Obviously this would require cargo add to update Cargo.lock which I'm unsure if it's acceptable.

I'm raising it here because I suspect it should be the default behavior (opt-out, not opt-in).

I disagree. In particular, “avoiding compatibility problems” only makes sense if the transitive B dependency is a public dependency of crate A anyways, and the public vs. private dependency distinction is not something that cargo can automatically detect/understand. And private dependencies are, well, private not only because someone arbitrarily decided to arbitrarily use the “public/private” terminology here to distinguish two things that need to be distinguished when you talk about semver-compatibility, but also because they’re actually supposed to be private in the sense of encapsulation, where private dependencies should not affect the users of a crate.

I imagine that it could get very annoying for crate maintainers (of a crate A with a private dependency on crate B) being annoyed by users “please update your B dependency because it negatively affects my cargo add behavior”.

2 Likes

Not yet, anyway:

Though the feature as implementated on nightly is still rather incomplete and is likely stalled until/if pubgrub is integrated as the version solver.

2 Likes