feat(add): suggest similarly named features #15438
Conversation
rustbot
added
Command-add
S-waiting-on-review
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? |
| .into_iter() | ||
| .format("\n ") | ||
| )?; | ||
| } | ||
| if deactivated.len() <= MAX_FEATURE_PRINTS { |
There was a problem hiding this comment.
Should we still display all of the features if there is a closest match for every unknown feature?
There was a problem hiding this comment.
I would say yes, although this might be pretty lengthy
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 presentSo 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?
src/cargo/ops/cargo_add/mod.rs
Outdated
| if !deactivated.is_empty() { | ||
| let mut suggested_features = Vec::new(); |
There was a problem hiding this comment.
Should we check even among activated features?
There was a problem hiding this comment.
Perhaps, but I don't see value in it: these features are already activated 🤷♂️
There was a problem hiding this comment.
The user might not realize its activated and they may be taking a feature that is indirectly activated and explicitly activating it.
There was a problem hiding this comment.
The user might not realize its activated and they may be taking a feature that is indirectly activated and explicitly activating it.
Sounds reasonable. So, do I have to check in activated features as well?
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
force-pushed
the
suggest_features
branch
from
32330f6 to
992abc3
Compare
Alright, done |
|
Ohh, just noticed that |
I think we have to rollback to |
|
Also, how should I approach fixing other tests? Some of them fail because output of |
|
https://github.com/rust-lang/cargo/actions/runs/14540352012/job/40796970702#step:13:4593 Set the environment variable |
Jujumba
force-pushed
the
suggest_features
branch
from
992abc3 to
1d8f58e
Compare
What does this PR try to resolve?
Fixes #15436
How should we test and review this PR?
There are 3 tests for each test case: