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?

src/cargo/ops/cargo_add/mod.rs

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

src/cargo/ops/cargo_add/mod.rs

+                                .into_iter()
+                                .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.

src/cargo/ops/cargo_add/mod.rs

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

tests/testsuite/cargo_add/feature_suggestion_multiple/stderr.term.svg

+    <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

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

src/cargo/ops/cargo_add/mod.rs Outdated Show resolved Hide resolved

epage reviewed Apr 18, 2025

View reviewed changes

src/cargo/ops/cargo_add/mod.rs Outdated Show resolved Hide resolved

test(add): add tests for feature suggestions

16c5947

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

feat(add): suggest similarly named features and update tests

86efb30

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 added 2 commits April 28, 2025 20:07

fix: output only one blank line between suggestions

1e7e209

feat: suggest activated features as well

bd5f6fe

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(add): suggest similarly named features #15438

feat(add): suggest similarly named features #15438

Jujumba commented Apr 18, 2025

rustbot commented Apr 18, 2025

epage commented Apr 18, 2025

epage Apr 18, 2025

Jujumba Apr 18, 2025

epage Apr 18, 2025

Jujumba Apr 19, 2025

epage Apr 28, 2025

Jujumba Apr 28, 2025

epage Apr 28, 2025

epage Apr 28, 2025

Jujumba commented Apr 18, 2025

epage commented Apr 18, 2025

Jujumba commented Apr 18, 2025

Jujumba commented Apr 18, 2025

Jujumba commented Apr 18, 2025 •

edited

Loading

Jujumba commented Apr 18, 2025

weihanglo commented Apr 18, 2025

Jujumba commented Apr 27, 2025

epage Apr 28, 2025

Jujumba Apr 28, 2025 •

edited

Loading

epage Apr 28, 2025

Jujumba commented Apr 28, 2025

epage commented Apr 28, 2025

Jujumba commented Apr 28, 2025

feat(add): suggest similarly named features #15438

Are you sure you want to change the base?

feat(add): suggest similarly named features #15438

Conversation

Jujumba commented Apr 18, 2025

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Apr 18, 2025

epage commented Apr 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jujumba commented Apr 18, 2025

epage commented Apr 18, 2025

Jujumba commented Apr 18, 2025

Jujumba commented Apr 18, 2025

Jujumba commented Apr 18, 2025 • edited Loading

Jujumba commented Apr 18, 2025

weihanglo commented Apr 18, 2025

Jujumba commented Apr 27, 2025

Choose a reason for hiding this comment

Jujumba Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jujumba commented Apr 28, 2025

epage commented Apr 28, 2025

Jujumba commented Apr 28, 2025

Jujumba commented Apr 18, 2025 •

edited

Loading

Jujumba Apr 28, 2025 •

edited

Loading