feat(add): suggest similarly named features - #15438

Jujumba · 2025-04-18T18:06:14Z

What does this PR try to resolve?

How should we test and review this PR?

There are 3 tests for each test case:

there are no feature suggestions
there's only one feature suggestion (most common)
there are several feature suggestions

rustbot · 2025-04-18T18:06:19Z

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

epage · 2025-04-18T18:13:09Z

There are 3 tests for each test case:

Would you be willing to restructure this to match the recommendation at https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request to add tests in a commit before your change with them passing and then your commit with your change updates the tests so they still pass so that the diff between the commits highlights the change in behavior?

epage · 2025-04-18T18:14:31Z

+                                .format("\n    ")
+                        )?;
+                    }
                    if deactivated.len() <= MAX_FEATURE_PRINTS {


Should we still display all of the features if there is a closest match for every unknown feature?

I would say yes, although this might be pretty lengthy

Looking around at how we do this elsewhere

Most places do not list all options, even without a suggestion

One place always lists out how to see all options, independent of whether there was a suggestion

2 places will conditionally show all options if a suggestion is not present

So there is only a small precedence but it tends to be towards not listing everything if there is a suggestion.

Thinking particularly for this case, we most likely want to focus the users attention on the suggestion. Also, since there was a concern over the number of suggestions, what there can be a lot more of is number of features. While we use columns, it can be quite large.

I lean towards only showing the suggestion.

Looking around at how we do this elsewhere

* Most places do not list all options, even without a suggestion * One place always lists out how to see all options, independent of whether there was a suggestion * 2 places will conditionally show all options if a suggestion is not present

So there is only a small precedence but it tends to be towards not listing everything if there is a suggestion.

Thinking particularly for this case, we most likely want to focus the users attention on the suggestion. Also, since there was a concern over the number of suggestions, what there can be a lot more of is number of features. While we use columns, it can be quite large.

I lean towards only showing the suggestion.

So, how should the output look like?

I'm not too sure what you are looking for with this question. As I said, I lean towards only showing the suggestion.

I'm not too sure what you are looking for with this question. As I said, I lean towards only showing the suggestion.

Sorry for the confusion, I mean: how should the suggestion look like? Just say that there N similarly spelled features, or perhaps something else?

Also, closest_msg appends two line breaks into the middle of the message, so maybe I should use different function?

Sorry for the confusion, I mean: how should the suggestion look like? Just say that there N similarly spelled features, or perhaps something else?

Unless we have a good reason, we should be use closest_msg. If we do have a good reason, we should mirror it. So that means we don't say there are "N similarly spelled features".

Also, closest_msg appends two line breaks into the middle of the message, so maybe I should use different function?

I commented on that at
#15438 (comment) . Other messages don't show two blank lines.

To double check, earlier in this thread, I asked about not showing deactivated or activated features. Unless I'm misunderstanding something, it looks like that isn't done yet.

To double check, earlier in this thread, I asked about not showing deactivated or activated features. Unless I'm misunderstanding something, it looks like that isn't done yet.

Looks like I missed it. (De)activated features shouldn't be printed only if there are feature suggestions, right?

Yes, that would align with other places that only exhaustively list things when there are no suggestions.

Jujumba · 2025-04-18T18:17:31Z

There are 3 tests for each test case:

Would you be willing to restructure this to match the recommendation at https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request to add tests in a commit before your change with them passing and then your commit with your change updates the tests so they still pass so that the diff between the commits highlights the change in behavior?

Sure! To make tests pass before I introduce my changes, I have to leave them blank, right?

epage · 2025-04-18T18:19:08Z

Sure! To make tests pass before I introduce my changes, I have to leave them blank, right?

In this case, they should still be doing adds; just there will be no suggestions given. If there are questions, the link I gave has links to example PRs.

Jujumba · 2025-04-18T19:06:59Z

Sure! To make tests pass before I introduce my changes, I have to leave them blank, right?

In this case, they should still be doing adds; just there will be no suggestions given. If there are questions, the link I gave has links to example PRs.

Alright, done

Jujumba · 2025-04-18T19:18:52Z

Ohh, just noticed that edit_distance::closest_msg appends two '\n' at the begging of the string, messing up the output

Jujumba · 2025-04-18T19:22:26Z

Ohh, just noticed that edit_distance::closest_msg appends two '\n' at the begging of the string, messing up the output

I think we have to rollback to edit_distance::closest or add the help message at the end

@epage

Jujumba · 2025-04-18T22:18:43Z

Also, how should I approach fixing other tests? Some of them fail because output of cargo add changed

weihanglo · 2025-04-18T22:39:12Z

https://github.com/rust-lang/cargo/actions/runs/14540352012/job/40796970702#step:13:4593

Set the environment variable SNAPSHOTS=overwrite. See https://doc.crates.io/contrib/tests/writing.html#updating-snapshots

Jujumba · 2025-04-27T20:47:03Z

@rustbot ready

epage · 2025-04-28T14:04:59Z

+    <tspan x="10px" y="118px"><tspan>help: a feature with a similar name exists: `bar`</tspan>
+</tspan>
+    <tspan x="10px" y="136px">
+</tspan>
+    <tspan x="10px" y="154px">
+</tspan>


We're showing two blank lines.

This may be an artifact of how closest_msg expects error message newlines to be handled. Could you do a refactor before this commit so that this commit only shows one visible line?

I'm not quite sure: do you mean removing blank lines or just leaving only one blank line per suggestion?

Sorry if I'm asking too much questions 😅

There shouldn't be two blank lines between items, only one.

This is likely an outcome of other error messages not being expected to have a trailing newline, so closest_msg adds it and then adds a blank line. But since we have a trailing newline, it makes two blank lines.

We likely need to just these messages to not have a trailing newline.

Jujumba · 2025-04-28T18:57:28Z

@rustbot ready

epage · 2025-04-28T19:19:45Z

I know it can sometimes be a pain to clean up commits but could you

Pull out the change in newlines to be before any future work was done
Squash the feature work

That'll be a big help for reviewing

Jujumba · 2025-04-28T20:40:49Z

I know it can sometimes be a pain to clean up commits but could you

Pull out the change in newlines to be before any future work was done

Squash the feature work

That'll be a big help for reviewing

Sure, I'll ping you once done

Jujumba · 2025-04-29T22:36:41Z

Hopefully the commit history is acceptable now

@rustbot ready

epage · 2025-04-30T13:58:13Z

Hopefully the commit history is acceptable now

Looks like you got rid of the leading test commits which make it easier to see how the behavior changed with each commit.

epage · 2025-04-30T13:59:05Z

-    <tspan x="10px" y="82px">
+    <tspan x="10px" y="82px"><tspan>no features available for crate my-package</tspan>
 </tspan>
-    <tspan x="10px" y="100px"><tspan>no features available for crate my-package</tspan>
-</tspan>
-    <tspan x="10px" y="118px">


Why did the previous commit add a blank line for this one to remove it?

epage · 2025-04-30T13:59:29Z

+    <tspan x="10px" y="64px"><tspan class="fg-red bold">error</tspan><tspan class="bold">:</tspan><tspan> unrecognized feature for crate my-package: none_existent</tspan>
+</tspan>
+    <tspan x="10px" y="82px"><tspan>disabled features:</tspan>
+</tspan>


Didn't the previous commit add a blank line between these?

Jujumba · 2025-04-30T17:12:26Z

Hopefully the commit history is acceptable now

Looks like you got rid of the leading test commits which make it easier to see how the behavior changed with each commit.

Sorry, I thought that I needed to squash commits regarding the actual feature into. Do you want me to undo all of this?..

epage · 2025-04-30T18:44:33Z

Since it looks like there are other changes to be made, it would be nice.

epage

Thanks!

Update cargo 25 commits in 7918c7eb59614c39f1c4e27e99d557720976bdd7..056f5f4f3c100cb36b5e9aed2d20b9ea70aae295 2025-04-27 09:44:23 +0000 to 2025-05-09 14:54:18 +0000 - Revert "doc: Mention `XDG_DATA_HOME`" (rust-lang/cargo#15512) - docs: update version notice for deprecation removal (rust-lang/cargo#15511) - doc: Update instructions on using native-completions (rust-lang/cargo#15480) - feat(network): use Retry-After header for HTTP 429 responses (rust-lang/cargo#15463) - CI: Require schema job to pass (rust-lang/cargo#15504) - chore(config): migrate renovate config (rust-lang/cargo#15501) - Make cargo script ignore workspaces (rust-lang/cargo#15496) - fix(rustc): Don't panic on unknown bins (rust-lang/cargo#15497) - test: Remove unused nightly requirements (rust-lang/cargo#15498) - Add support for `-Zembed-metadata` (rust-lang/cargo#15378) - Fix tracking issue template link (rust-lang/cargo#15494) - Refactor artifact deps in FeatureResolver::deps (rust-lang/cargo#15492) - Improved error message for versions prefixed with `v` (rust-lang/cargo#15484) - chore: fix some typos in comment (rust-lang/cargo#15485) - fix: default to all targets when using `--edition` and ` --edition-idioms` in cargo fix (rust-lang/cargo#15192) - Update fingerprint footnote (rust-lang/cargo#15478) - feat(add): suggest similarly named features (rust-lang/cargo#15438) - In package-workspace, keep dev-dependencies if they have a version (rust-lang/cargo#15470) - docs: fix a typo in DependencyUI (rust-lang/cargo#15472) - fix grammar, and remove confusing example (rust-lang/cargo#15457) - Added tracing spans for rustc invocations (rust-lang/cargo#15464) - Trivial tweaks to 'target_short_hash' (rust-lang/cargo#15461) - chore(deps): update msrv (3 versions) to v1.84 (rust-lang/cargo#15456) - feat(add/install): check if given crate argument would be valid with inserted @ symbol (rust-lang/cargo#15441) - chang 1 tries to 1 try (rust-lang/cargo#15328) r? ghost

rustbot assigned epage Apr 18, 2025

rustbot added Command-add S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2025

epage reviewed Apr 18, 2025

View reviewed changes

Comment thread src/cargo/ops/cargo_add/mod.rs Outdated

epage reviewed Apr 18, 2025

View reviewed changes

Comment thread src/cargo/ops/cargo_add/mod.rs Outdated

Jujumba force-pushed the suggest_features branch from 32330f6 to 992abc3 Compare April 18, 2025 19:06

Jujumba requested a review from epage April 18, 2025 19:07

Jujumba force-pushed the suggest_features branch from 992abc3 to 1d8f58e Compare April 19, 2025 09:47

epage reviewed Apr 28, 2025

View reviewed changes

Jujumba force-pushed the suggest_features branch from 1d8f58e to bd5f6fe Compare April 28, 2025 18:18

Jujumba requested a review from epage April 28, 2025 18:57

Jujumba force-pushed the suggest_features branch 2 times, most recently from f4c98f3 to ce7d1fb Compare April 29, 2025 22:03

epage reviewed Apr 30, 2025

View reviewed changes

fix: output a blank line between items

49e2f84

Jujumba added 2 commits May 1, 2025 16:02

feat: add tests for similarly named features

e29810c

feat: suggest similarly named features

ba494bc

Jujumba force-pushed the suggest_features branch from ce7d1fb to ba494bc Compare May 1, 2025 14:13

epage approved these changes May 1, 2025

View reviewed changes

epage added this pull request to the merge queue May 1, 2025

Merged via the queue into rust-lang:master with commit 64e3cd1 May 1, 2025

rustbot mentioned this pull request May 1, 2025

Print all features when the verbose flag is set #15375

Open

weihanglo mentioned this pull request May 9, 2025

Update cargo rust-lang/rust#140876

Merged

rustbot added this to the 1.89.0 milestone May 10, 2025

epage mentioned this pull request Jun 13, 2025

feat: always show list of features on cargo add #15658

Closed

Uh oh!

Uh oh!

Conversation

Jujumba commented Apr 18, 2025

What does this PR try to resolve?

How should we test and review this PR?

Uh oh!

rustbot commented Apr 18, 2025

Uh oh!

epage commented Apr 18, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jujumba commented Apr 18, 2025

Uh oh!

epage commented Apr 18, 2025

Uh oh!

Jujumba commented Apr 18, 2025

Uh oh!

Jujumba commented Apr 18, 2025

Uh oh!

Jujumba commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jujumba commented Apr 18, 2025

Uh oh!

weihanglo commented Apr 18, 2025

Uh oh!

Jujumba commented Apr 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jujumba Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jujumba commented Apr 28, 2025

Uh oh!

epage commented Apr 28, 2025

Uh oh!

Jujumba commented Apr 28, 2025

Uh oh!

Jujumba commented Apr 29, 2025

Uh oh!

epage commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jujumba commented Apr 30, 2025

Uh oh!

epage commented Apr 30, 2025

Uh oh!

epage left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Jujumba commented Apr 18, 2025 •

edited

Loading

Jujumba Apr 28, 2025 •

edited

Loading