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

tags: Separate versions with multidigit components with '_' #240

Merged
merged 3 commits into from
Feb 8, 2020
Merged

tags: Separate versions with multidigit components with '_' #240

merged 3 commits into from
Feb 8, 2020

Conversation

jwodder
Copy link
Contributor

@jwodder jwodder commented Nov 22, 2019

PEP 425 defines the version string used in Python implementation tags as follows:

The version is py_version_nodot. CPython gets away with no dot, but if one is needed the underscore _ is used instead.

I take this to mean that, starting with Python 3.10, Python implementation tags will be of the form cp3_10 instead of cp310. This PR thus updates tags.py to insert underscores in version strings in tags whenever one or more of the version's components are at least 10.

Note: I am assuming that the sysconfig var py_version_nodot will be updated to use an underscore when Python 3.10 is released; if it is not, then the definition of interpreter_version() will have to be changed to use something other than py_version_nodot.

@brettcannon
Copy link
Member

@jwodder just an FYI this PR is going to have massive merge conflicts once #231 lands (which should hopefully be in the very near future).

I also don't know yet about the proposed change. There hasn't been a discussion about how to handle Python 3.10 so I don't know how we may want to solve it. It may be that ripping out any custom version number specification for CPython is best and to just entirely rely on nodot, but that would require checking how compatible that is across OSs and Python versions.

@jwodder
Copy link
Contributor Author

jwodder commented Dec 1, 2019

PR updated for #231.

@pradyunsg pradyunsg changed the title tags.py: Separate versions with multidigit components with '_' tags: Separate versions with multidigit components with '_' Dec 12, 2019
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Just some ideas on how to simplify the tests.

@@ -543,7 +553,11 @@ def test_wide_unicode(self, unicode_size, maxunicode, version, result, monkeypat
config = {"Py_DEBUG": 0, "WITH_PYMALLOC": 0, "Py_UNICODE_SIZE": unicode_size}
monkeypatch.setattr(sysconfig, "get_config_var", config.__getitem__)
monkeypatch.setattr(sys, "maxunicode", maxunicode)
base_abi = "cp{}{}".format(version[0], version[1])
if version[0] >= 10 or version[1] >= 10:
Copy link
Member

Choose a reason for hiding this comment

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

It's okay to call _version_nodot() in this case as you're not testing the version details, so no need to duplicate the logic here.

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 tests now use _version_nodot() where applicable.

@@ -596,7 +643,11 @@ def test_all_args(self):

def test_python_version_defaults(self):
tag = next(tags.cpython_tags(abis=["abi3"], platforms=["any"]))
interpreter = "cp{}{}".format(*sys.version_info[:2])
if sys.version_info[0] >= 10 or sys.version_info[1] >= 10:
Copy link
Member

Choose a reason for hiding this comment

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

You can call _version_nodot() directly as the test is just making sure the result is based on sys.version_info and is formatted appropriately.

tests/test_tags.py Show resolved Hide resolved
@@ -806,13 +955,23 @@ def test_windows_cpython(self, mock_interpreter_name, monkeypatch):
abis = tags._cpython_abis(sys.version_info[:2])
platforms = tags._generic_platforms()
result = list(tags.sys_tags())
interpreter = "cp{major}{minor}".format(
major=sys.version_info[0], minor=sys.version_info[1]
if sys.version_info[0] >= 10 or sys.version_info[1] >= 10:
Copy link
Member

Choose a reason for hiding this comment

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

For the cases where it's worth keeping the logic it should probably be broken out into its own test function. Although honestly unless the test is specifically for formatting and thus has hard-coded expectations I say just call _version_nodot() directly.

@brettcannon
Copy link
Member

@jwodder BTW when you're ready for another review just refresh the review request and it will show up in my GitHub PR review queue.

@brettcannon brettcannon merged commit 318a034 into pypa:master Feb 8, 2020
@brettcannon
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants