Raise error if --user and --target arguments are used together #7777
Conversation
|
@pradyunsg please suggest a good error message. I am really bad with this. |
sinscary
force-pushed
the
fix_#7249
branch
2 times, most recently
from
4ed5019
to
b6d775d
Compare
|
@pradyunsg can you review the changes? |
src/pip/_internal/cli/main_parser.py
Outdated
| cmd_args.remove(cmd_name) | ||
|
|
||
| return cmd_name, cmd_args | ||
| if set(['--user', '--target']).issubset(set(args_else)): |
There was a problem hiding this comment.
I would handle this in InstallCommand instead, since it is the only context where --user and --target are mutually exclusive.
It is much easier to handle it in run() as well; optparse documentation conveniently provides an example; you can simply write something like
def run(self, options, args):
if options.use_user_site and options.target_dir is not None:
raise CommandError('--user and --target cannot be used together.')
# ...There was a problem hiding this comment.
@uranusjr My bad. I didn't realise --target was being used with install command only. But don't you think error out early would be better than executing all the code to populate the options, import install module etc. and then error it out. I mean we already know these both args are not allowed together.
There was a problem hiding this comment.
I mean we already know these both args are not allowed together.
Indeed, but you’re already missing -t which is an alias of --target. And what happens if we add more aliases to either argument? Populating the whole options is actually a good thing IMO since it provides both consistency and safety.
There was a problem hiding this comment.
Oh yeah that makes sense, Looks like I didn't consider all the cases 😞. Well, will make the changes. Thanks for the review 😄
There was a problem hiding this comment.
As we are on the topic. Can you suggest a better error message to show?
There was a problem hiding this comment.
That I have absolutely no suggestions. I think the current message is good enough, but I’m terrible at this 😆
|
@uranusjr @pradyunsg Can you review the changes again? |
news/7249.feature
Outdated
| @@ -0,0 +1 @@ | |||
| Raise error if --user and --target are used together in command | |||
|
One nitpicking, otherwise LGTM 👍 |
|
@uranusjr done. But Looks like this test |
|
@uranusjr @pradyunsg meanwhile tests run, just curious to know why does pip have so many checks 😕 ? 45 checks, I mean that's a lot compared to other pypa projects. |
lock
bot
added
the
auto-locked
closes #7249
This PR adds a validation which runs while parsing the command and throws a
CommandErrorif--userand--targetargs are used together in same command.