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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/6585.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Allow ``--python-version`` to be passed as a dotted version string (e.g.
``3.7`` or ``3.7.3``).
57 changes: 42 additions & 15 deletions src/pip/_internal/cli/cmdoptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from distutils.util import strtobool
from functools import partial
from optparse import SUPPRESS_HELP, Option, OptionGroup
from textwrap import dedent

from pip._internal.exceptions import CommandError
from pip._internal.locations import USER_CACHE_DIR, src_prefix
Expand Down Expand Up @@ -480,24 +481,49 @@ def only_binary():

# This was made a separate function for unit-testing purposes.
def _convert_python_version(value):
# type: (str) -> Tuple[int, ...]
# type: (str) -> Tuple[Tuple[int, ...], Optional[str]]
"""
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 :)

non-None if and only if there was a parsing error.
"""
if len(value) == 1:
parts = [value]
else:
parts = [value[0], value[1:]]
if not value:
# The empty string is the same as not providing a value.
return (None, None)

parts = value.split('.')
if len(parts) > 3:
return ((), 'at most three version parts are allowed')

if len(parts) == 1:
# Then we are in the case of "3" or "37".
value = parts[0]
if len(value) > 1:
parts = [value[0], value[1:]]

try:
version_info = tuple(int(part) for part in parts)
except ValueError:
return ((), 'each version part must be an integer')

return tuple(int(part) for part in parts)
return (version_info, None)


def _handle_python_version(option, opt_str, value, parser):
# type: (Option, str, str, OptionParser) -> None
"""
Convert a string like "3" or "34" into a tuple of ints.
Handle a provided --python-version value.
"""
version_info = _convert_python_version(value)
version_info, error_msg = _convert_python_version(value)
if error_msg is not None:
msg = (
'invalid --python-version value: {!r}: {}'.format(
value, error_msg,
)
)
raise_option_error(parser, option=option, msg=msg)

parser.values.python_version = version_info


Expand All @@ -509,12 +535,13 @@ def _handle_python_version(option, opt_str, value, parser):
action='callback',
callback=_handle_python_version, type='str',
default=None,
help=("Only use wheels compatible with Python "
"interpreter version <version>. If not specified, then the "
"current system interpreter minor version is used. A major "
"version (e.g. '2') can be specified to match all "
"minor revs of that major version. A minor version "
"(e.g. '34') can also be specified."),
help=dedent("""\
The Python interpreter version to use for wheel and "Requires-Python"
compatibility checks. Defaults to a version derived from the running
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

) # type: Callable[..., Option]


Expand Down
19 changes: 14 additions & 5 deletions tests/unit/test_cmdoptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,21 @@


@pytest.mark.parametrize('value, expected', [
('2', (2,)),
('3', (3,)),
('34', (3, 4)),
('', (None, None)),
('2', ((2,), None)),
('3', ((3,), None)),
('3.7', ((3, 7), None)),
('3.7.3', ((3, 7, 3), None)),
# Test strings without dots of length bigger than 1.
('34', ((3, 4), None)),
# Test a 2-digit minor version.
('310', (3, 10)),
('310', ((3, 10), None)),
# Test some values that fail to parse.
('ab', ((), 'each version part must be an integer')),
('3a', ((), 'each version part must be an integer')),
('3.7.a', ((), 'each version part must be an integer')),
('3.7.3.1', ((), 'at most three version parts are allowed')),
])
def test_convert_python_version(value, expected):
actual = _convert_python_version(value)
assert actual == expected
assert actual == expected, 'actual: {!r}'.format(actual)