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

Allow dotted version strings for --python-version. #6585

Merged
merged 1 commit into from
Jun 10, 2019

Conversation

cjerdonek
Copy link
Member

As suggested in this comment by @xavfernandez, this PR adds support for passing dotted version strings to --python-version: #6371 (comment)

Currently, only major or major-minor version strings like 3 or 37 are supported (with no dot).

@cjerdonek cjerdonek added C: download About fetching data from PyPI and other sources type: enhancement Improvements to functionality labels Jun 9, 2019
@cjerdonek cjerdonek force-pushed the python-version-support-dotted branch from 88f7e06 to 85dc783 Compare June 9, 2019 02:45
@cjerdonek cjerdonek force-pushed the python-version-support-dotted branch from 85dc783 to 4bb225d Compare June 9, 2019 02:59
Convert a string like "3" or "34" into a tuple of ints.
Convert a version string like "3", "37", or "3.7.3" into a tuple of ints.

:return: A 2-tuple (version_info, error_msg), where `error_msg` is
Copy link
Member

Choose a reason for hiding this comment

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

This seems strange, why not raise in such case ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is raised by the caller using raise_option_error() and the message returned by this function. It's easier to test this way because raise_option_error() requires both an Option and OptionParser object, and it seems like it would be an unnecessary complication to raise a ValueError just so the caller can catch it, get the message from the exception's argument, and then re-raise using raise_option_error(). (This function is used in only one place and was separated out only to make testing the --python-version argument handling inside _handle_python_version() easier.)

Copy link
Member

@xavfernandez xavfernandez Jun 10, 2019

Choose a reason for hiding this comment

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

It's easier to test this way

I suspected so but this return (result, error) feels strange in a python program (and looks more like golang)...
Raising an Exception would be expected (and could be tested with an assertRaise), so I guess it is a matter of taste :)

interpreter. The version can be specified using up to three dot-separated
integers (e.g. "3" for 3.0.0, "3.7" for 3.7.0, or "3.7.3"). A major-minor
version can also be given as a string without dots (e.g. "37" for 3.7.0).
"""),
Copy link
Member

Choose a reason for hiding this comment

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

I thought we'd want to deprecate the 37 values in favor of the unambiguous 3.7 ?

Copy link
Member Author

@cjerdonek cjerdonek Jun 9, 2019

Choose a reason for hiding this comment

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

Can that be done as a separate PR? I was hoping it could be done as part of a separate PR / discussion. I usually prefer keeping PR's smaller when possible to make it easier for people to review and discuss changes, etc (and code them :) ).

Copy link
Member

Choose a reason for hiding this comment

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

Several PRs are fine by me

Convert a string like "3" or "34" into a tuple of ints.
Convert a version string like "3", "37", or "3.7.3" into a tuple of ints.

:return: A 2-tuple (version_info, error_msg), where `error_msg` is
Copy link
Member

@xavfernandez xavfernandez Jun 10, 2019

Choose a reason for hiding this comment

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

It's easier to test this way

I suspected so but this return (result, error) feels strange in a python program (and looks more like golang)...
Raising an Exception would be expected (and could be tested with an assertRaise), so I guess it is a matter of taste :)

interpreter. The version can be specified using up to three dot-separated
integers (e.g. "3" for 3.0.0, "3.7" for 3.7.0, or "3.7.3"). A major-minor
version can also be given as a string without dots (e.g. "37" for 3.7.0).
"""),
Copy link
Member

Choose a reason for hiding this comment

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

Several PRs are fine by me

@cjerdonek
Copy link
Member Author

Okay, thanks, @xavfernandez.

@cjerdonek cjerdonek merged commit 1485193 into pypa:master Jun 10, 2019
@cjerdonek cjerdonek deleted the python-version-support-dotted branch June 10, 2019 18:01
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jul 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 10, 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 C: download About fetching data from PyPI and other sources type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants