-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: warning --package
will be ignore if --workspace
presents
#15071
base: master
Are you sure you want to change the base?
Conversation
r? @weihanglo rustbot has assigned @weihanglo. Use |
src/bin/cargo/commands/add.rs
Outdated
@@ -185,7 +185,7 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult { | |||
print_available_packages(&ws)?; | |||
} | |||
|
|||
let packages = args.packages_from_flags()?; | |||
let packages = args.packages_from_flags(gctx, false)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set ignore_package as true to indicate that dont pop-up warnning, these commands needs this
why does add
not need this?
src/bin/cargo/commands/remove.rs
Outdated
@@ -66,7 +66,7 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult { | |||
print_available_packages(&workspace)?; | |||
} | |||
|
|||
let packages = args.packages_from_flags()?; | |||
let packages = args.packages_from_flags(gctx, false)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set ignore_package as true to indicate that dont pop-up warnning, these commands needs this
why does remove
not need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add
and remove
command don't support the --workspace option.
tests/testsuite/workspaces.rs
Outdated
// baz is present in the workspace and it is a real existing package. | ||
p.cargo("check --package baz --workspace") | ||
.with_stderr_data(str![[r#" | ||
[CHECKING] foo v0.1.0 ([ROOT]/foo) | ||
[WARNING] ignoring `--package` as the `--workspace` or `--all` flag is set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue wasn't about redundant --package
flags but about non-existent packages. If we decide to warn on redundant flags, then we can simplify the design of this but that is something that needs to be figured out first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue wasn't about redundant --package flags but about non-existent package.
My PR was deviated from the original question. I tried to add warning for non-existent package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have a counter proposal, that would ideally be handled in the issue. Let's continue the discussion there
28d534b
to
a407aeb
Compare
a407aeb
to
03b3ca7
Compare
What does this PR try to resolve?
Fixes #12978
This PR trigger warning if the specified package is non-existence
How should we test and review this PR?
one commit add test, one commit fixes the issue.
Additional information
setignore_package
as true to indicate that dont pop-up warnning, these commands needs thiscargo addcargo remove