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

Warn if built-in subcommand aliases shadow user-defined aliases #6222

Closed
wants to merge 5 commits into from

Conversation

ordovicia
Copy link
Contributor

This PR makes Cargo emit a warning if built-in subcommand aliases (b, r, and t) shadow user-defined aliases.

$ cat .cargo/config
[alias]
b = "foo"

$ ./target/debug/cargo b
warning: alias `b` is ignored, because it is shadowed by a built-in alias for `build`

In order to handle subcommand aliases after clap parses the command line options, it seems to be necessary to stop using clap for subcommand aliases.
So now help command does not work with subcommand aliases, but I think it is not a big problem.

Fixes #6221

@rust-highfive
Copy link

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

dwijnand
dwijnand previously approved these changes Oct 26, 2018
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ehuss
Copy link
Contributor

ehuss commented Oct 26, 2018

If the goal is to be able to add a new alias (c) without clobbering existing user aliases, then shouldn't user-aliases take precedence over builtin aliases?

@ordovicia
Copy link
Contributor Author

ordovicia commented Oct 26, 2018

Umm..., sorry for confusing.
My goal is to add a new built-in alias (#6218), as you know, but I think it is not absolutely necessary to maintain the perfect compatibility.
In the current version of Cargo, user-defined aliases cannot override built-in ones, right?
I do not think it is good to change this behavior, or to introduce inconsistency.

BTW, the AppVeyor failure seems to be spurious (timed out).

@alexcrichton
Copy link
Member

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned matklad Oct 27, 2018
@dwijnand dwijnand dismissed their stale review October 31, 2018 20:43

I agree with @ehuss, this should be the other way round.

@ehuss
Copy link
Contributor

ehuss commented Nov 2, 2018

@ordovicia sorry for the delay. I think it would be best if user aliases took precedence over built-in ones. That would allow us to safely add aliases without disrupting existing user configurations. I think this would require not using clap aliases and instead checking them manually near where user aliases are checked.

@ordovicia
Copy link
Contributor Author

Thank you all. I am convinced.
I will make a new PR that allows user aliases take precedence over built-in ones.

@ordovicia ordovicia closed this Nov 4, 2018
@ordovicia ordovicia deleted the subcmd-alias-shadow branch November 4, 2018 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants