Fix subcommands accepting --version - #14231
Conversation
|
Do we need a test here? |
2ykwang
left a comment
There was a problem hiding this comment.
the root cause seems to be that --version is defined in the shared options group
https://github.com/pypa/pip/blob/main/src/pip/_internal/cli/cmdoptions.py#L1355-L1385
| ) | ||
| options.cache_dir = None | ||
|
|
||
| if options.version: |
There was a problem hiding this comment.
even with this check, --version still shows up in the subcommand's --help.
There was a problem hiding this comment.
yes, it stays in subcommands since --version lives in general_group. I already tried removing it from there for the subcommand, but that broke some tests. Maybe there are other options
There was a problem hiding this comment.
done.
found a way to suppress the hint without removing it :)
There was a problem hiding this comment.
One thing that feels a bit off to me.
current implementation adds special-case handling just for --version. If possible, wouldn't it be better to avoid this kind of special case?
--version is only meaningful on the top-level pip command. So rather than keeping it in general_group, defining and handling it only in the top-level parser seems like a more natural structure to me.
That way:
--versionno longer shows up inpip <subcommand> --help- there's no need for separate special-case handling anymore
- and future changes related to
--versioncan be handled consistently at the parser level.
There was a problem hiding this comment.
I already tried removing it from there for the subcommand, but that broke some tests. Maybe there are other options
My guess is the broken tests came from removing version from general_group. pip --version itself could no longer be handled. Looking at the code where pip processes the top-level command might help.
There was a problem hiding this comment.
One thing that feels a bit off to me.
current implementation adds special-case handling just for
--version. If possible, wouldn't it be better to avoid this kind of special case?
--versionis only meaningful on the top-level pip command. So rather than keeping it ingeneral_group, defining and handling it only in the top-level parser seems like a more natural structure to me.That way:
* `--version` no longer shows up in `pip <subcommand> --help` * there's no need for separate special-case handling anymore * and future changes related to `--version` can be handled consistently at the parser level.
Damn, yeah, agreed. Much better this way.
Found how to do it. I ll remove it from general_options and add it as an option for pip, instead
|
If possible, it would be nice to have a regression test for |
done |
What does this PR do?
pip wheel . --versiondoesn't exit #9456This PR forbids using
pip <command> --version, leaving the--versionoption only for pip. At the same time, if such command is still executed, an error will be raised:no such option: --versionExample:
PR Checklist: