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

Refactor argument parser construction #239

Merged
merged 1 commit into from
Oct 18, 2019
Merged

Conversation

clbarnes
Copy link
Contributor

Each subparser is added in its own function

@clbarnes
Copy link
Contributor Author

clbarnes commented Oct 16, 2019

This does not conflict with the pipxrc PR.

pipx/main.py Outdated
@@ -30,6 +31,7 @@


__version__ = "0.14.0.0"
__version_info__ = tuple(int(n) for n in __version__.split("."))
Copy link
Member

Choose a reason for hiding this comment

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

I sometimes make beta releases, such as "0.14.1.0b0". Can the int cast be omitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention of __version_info__ is to sort properly, which a double-digit string wouldn't do - if 4 segments and an optional beta segment is the only case to account for I don't mind making this logic more complete.

@clbarnes clbarnes force-pushed the argparse-refactor branch 3 times, most recently from bdf9ec1 to a97a88b Compare October 16, 2019 19:04
@cs01
Copy link
Member

cs01 commented Oct 17, 2019

I updated pipx's unit tests and some of pipx's code, but now there is a merge conflict in this PR. But it should be more straightforward to write tests now and measure coverage.

pipx/main.py Outdated
mkdir(constants.PIPX_LOCAL_VENVS)
mkdir(constants.LOCAL_BIN_DIR)
mkdir(constants.PIPX_VENV_CACHEDIR)
mkdir(PIPX_LOCAL_VENVS)
Copy link
Member

Choose a reason for hiding this comment

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

The tests patch the value of these constants, so they need the constants. prefix

pipx/main.py Outdated
Comment on lines 25 to 29
_, __version_info__, subver, *_ = parse_version(__version__)._key # type: ignore
if isinstance(subver, tuple):
while len(__version_info__) < 4:
__version_info__ += (0,)
__version_info__ += subver
Copy link
Member

Choose a reason for hiding this comment

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

would you mind adding a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Are we set on only allowing a version with 1 to 4 components, plus an optional a/b/c component?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it will always be something like 0.1.2.3.4, 0.1.2.3.4b0, 0.1.2.3.4rc0, etc.

Each subparser is added in its own function
dest="command", description="Get help for commands with pipx COMMAND --help"
)

def _add_install(subparsers):
Copy link
Member

Choose a reason for hiding this comment

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

can all these functions have types added to them?

@cs01
Copy link
Member

cs01 commented Oct 18, 2019

Looks good, thanks! I'll merge now and we can add types later if we want.

@cs01 cs01 merged commit 997b0eb into pypa:master Oct 18, 2019
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.

2 participants