Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support underline and shortline compatible for cargo-remove #14814

Closed
A4-Tacks opened this issue Nov 13, 2024 · 4 comments
Closed

Support underline and shortline compatible for cargo-remove #14814

A4-Tacks opened this issue Nov 13, 2024 · 4 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-remove S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@A4-Tacks
Copy link

Problem

Reproduction:

lrne@Acer-Lrne[/path/to/]
    $ cargo new foo && cd foo
    Creating binary (application) `foo` package
note: see more `Cargo.toml` keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
lrne@Acer-Lrne[/path/to/foo/]
    $ cargo add cfg_if
    Updating `rsproxy-sparse` index
warning: translating `cfg_if` to `cfg-if`
      Adding cfg-if v1.0.0 to dependencies
             Features:
             - compiler_builtins
             - core
             - rustc-dep-of-std
    Updating `rsproxy-sparse` index
     Locking 2 packages to latest compatible versions
lrne@Acer-Lrne[/path/to/foo/]
    $ cargo remove cfg_if
    Removing cfg_if from dependencies
error: the dependency `cfg_if` could not be found in `dependencies`
lrne@Acer-Lrne[/path/to/foo/]
    $ cargo remove cfg-if
    Removing cfg-if from dependencies
lrne@Acer-Lrne[/path/to/foo/]
    $ 

Proposed Solution

No response

Notes

No response

@A4-Tacks A4-Tacks added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Nov 13, 2024
@epage
Copy link
Contributor

epage commented Nov 13, 2024

At least providing a "did you mean" more generally and not just for word separators) in the error messages would be good.

However, I'm unsure how far we should take normalization in Cargo. Yes, we do it in cargo-add. I don't remember the reason it did this in cargo-edit and I think we overlooked discussing this when merging. While remove is parallel to add, its also like other commands in that you've now been warned that the name isn't correct and you are operating on your existing dependencies.

@epage epage added S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-triage Status: This issue is waiting on initial triage. labels Nov 13, 2024
@A4-Tacks
Copy link
Author

If cargo-remove cannot translating underline and shortline, at least try giving a similar package name hint in the error message

epage added a commit to epage/cargo that referenced this issue Nov 13, 2024
`cargo remove` already will suggest other tables if appropriate.
`cargo add` auto-corrects `_` and `-`.

This is an extension of the two by suggesting existing dependencies
within the same table that are "close".

I chose to make alt tables and alt deps mutually exclusive in the
message because I didn't wantto deal with how to separate things if both
show up.
Most likely, people will only expect one or the other and if its in an
alt table, then most likely they mean that.

Related to rust-lang#14814
github-merge-queue bot pushed a commit that referenced this issue Nov 14, 2024
### What does this PR try to resolve?

`cargo remove` already will suggest other tables if appropriate.
`cargo add` auto-corrects `_` and `-`.

This is an extension of the two by suggesting existing dependencies
within the same table that are "close".

Related to #14814

### How should we test and review this PR?

I chose to make alt tables and alt deps mutually exclusive in the
message because I didn't wantto deal with how to separate things if both
show up.
Most likely, people will only expect one or the other and if its in an
alt table, then most likely they mean that.

### Additional information
@epage
Copy link
Contributor

epage commented Nov 14, 2024

#14818 improved the error message.

@A4-Tacks
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-remove S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

2 participants