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

commented Jun 9, 2019

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 force-pushed the cjerdonek:python-version-support-dotted branch from 88f7e06 to 85dc783 Jun 9, 2019

@cjerdonek cjerdonek force-pushed the cjerdonek:python-version-support-dotted branch from 85dc783 to 4bb225d Jun 9, 2019

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

This comment has been minimized.

Copy link
@xavfernandez

xavfernandez Jun 9, 2019

Contributor

This seems strange, why not raise in such case ?

This comment has been minimized.

Copy link
@cjerdonek

cjerdonek Jun 9, 2019

Author Member

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.)

This comment has been minimized.

Copy link
@xavfernandez

xavfernandez Jun 10, 2019

Contributor

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).
"""),

This comment has been minimized.

Copy link
@xavfernandez

xavfernandez Jun 9, 2019

Contributor

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

This comment has been minimized.

Copy link
@cjerdonek

cjerdonek Jun 9, 2019

Author Member

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 :) ).

This comment has been minimized.

Copy link
@xavfernandez

xavfernandez Jun 10, 2019

Contributor

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

This comment has been minimized.

Copy link
@xavfernandez

xavfernandez Jun 10, 2019

Contributor

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).
"""),

This comment has been minimized.

Copy link
@xavfernandez

xavfernandez Jun 10, 2019

Contributor

Several PRs are fine by me

@cjerdonek

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

Okay, thanks, @xavfernandez.

@cjerdonek cjerdonek merged commit 1485193 into pypa:master Jun 10, 2019

29 checks passed

Linux Build #20190608.9 succeeded
Details
Linux (Package) Package succeeded
Details
Linux (Test Primary Python27) Test Primary Python27 succeeded
Details
Linux (Test Primary Python36) Test Primary Python36 succeeded
Details
Linux (Test Secondary Python34) Test Secondary Python34 succeeded
Details
Linux (Test Secondary Python35) Test Secondary Python35 succeeded
Details
Linux (Test Secondary Python37) Test Secondary Python37 succeeded
Details
Windows Build #20190608.9 succeeded
Details
Windows (Package) Package succeeded
Details
Windows (Test Primary Python27-x64) Test Primary Python27-x64 succeeded
Details
Windows (Test Primary Python36-x64) Test Primary Python36-x64 succeeded
Details
Windows (Test Secondary Python27-x86) Test Secondary Python27-x86 succeeded
Details
Windows (Test Secondary Python34-x64) Test Secondary Python34-x64 succeeded
Details
Windows (Test Secondary Python34-x86) Test Secondary Python34-x86 succeeded
Details
Windows (Test Secondary Python35-x64) Test Secondary Python35-x64 succeeded
Details
Windows (Test Secondary Python35-x86) Test Secondary Python35-x86 succeeded
Details
Windows (Test Secondary Python36-x86) Test Secondary Python36-x86 succeeded
Details
Windows (Test Secondary Python37-x64) Test Secondary Python37-x64 succeeded
Details
Windows (Test Secondary Python37-x86) Test Secondary Python37-x86 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
macOS Build #20190608.9 succeeded
Details
macOS (Package) Package succeeded
Details
macOS (Test Primary Python27) Test Primary Python27 succeeded
Details
macOS (Test Primary Python36) Test Primary Python36 succeeded
Details
macOS (Test Secondary Python34) Test Secondary Python34 succeeded
Details
macOS (Test Secondary Python35) Test Secondary Python35 succeeded
Details
macOS (Test Secondary Python37) Test Secondary Python37 succeeded
Details
news-file/pr News files updated and/or change is trivial.
Details

@cjerdonek cjerdonek deleted the cjerdonek:python-version-support-dotted branch Jun 10, 2019

@lock lock bot added the S: auto-locked 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.
Projects
None yet
2 participants
You can’t perform that action at this time.