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

Consolidate --path parsing into cmdoptions.py #6600

Merged
merged 1 commit into from
Jun 14, 2019

Conversation

lkollar
Copy link
Contributor

@lkollar lkollar commented Jun 12, 2019

This option is used both in the freeze and list commands. The code to parse the option and validate that it's not used with incompatible arguments should be done in one place.

This addresses @cjerdonek's comment in #6580.

@xavfernandez xavfernandez added skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code labels Jun 12, 2019
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

LGTM, depending on the answer to the ordering comment (and fixing the linter).

@@ -75,6 +70,7 @@ def __init__(self, *args, **kw):
action='store_true',
help='Exclude editable package from output.')

self.cmd_opts.add_option(cmdoptions.list_path())
Copy link
Member

Choose a reason for hiding this comment

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

Does where add_option() is called control the order of display when invoking --help? If so, would it make sense to group --path after --local and --user like it was before (since --path is mutually exclusive with those other options)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right. Fixed.

@lkollar lkollar force-pushed the consolidate-path-opt-parsing branch from 65d0f6a to ff47ff8 Compare June 13, 2019 08:35
This option is used both in the `freeze` and `list` commands. The code
to parse the option and validate that it's not used with incompatible
arguments should be done in one place.
@lkollar lkollar force-pushed the consolidate-path-opt-parsing branch from ff47ff8 to 246130c Compare June 13, 2019 08:37
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Great, thanks again!

@cjerdonek cjerdonek merged commit 3f8df24 into pypa:master Jun 14, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jul 14, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants