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

Fix for Python 3.10: don't compare to sys.version string #2076

Closed
wants to merge 1 commit into from

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Jan 15, 2020

When dealing with version numbers, it's generally safer to use the sys.version_info tuple that the sys.version string.

>>> sys.version
'3.7.5 (default, Nov  1 2019, 02:16:32) \n[Clang 11.0.0 (clang-1100.0.33.8)]'
>>> sys.version[:3]
'3.7'
>>> fake_version310_info = '3.10.5 (default, Nov  1 2019, 02:16:32) \n[Clang 11.0.0 (clang-1100.0.33.8)]'
>>> fake_version310_info[:3]
'3.1'

When Python 3.10 comes around (probably in May 2020 when Python 3.9 reaches beta), it will claim to be on Python 3.1!

In this case, Python 3.2 is no longer supported, so this check can be removed. (And in fact, it wouldn't have failed until Python 3.20, but it's good to clean up.)


Found with https://github.com/asottile/flake8-2020

@emmanuelle
Copy link
Contributor

Thank you @hugovk. I have no idea why we excluded python 3.2 here... any idea @nicolaskruchten @jonmmease ?

@hugovk
Copy link
Contributor Author

hugovk commented Jan 15, 2020

If it helps, it was added in PR #1476.

@hugovk hugovk mentioned this pull request Jan 15, 2020
@nicolaskruchten
Copy link
Contributor

No idea, but probably for a good reason, in which case I'd rather upgrade the check to use the right thing rather than removing it :)

@hugovk hugovk force-pushed the fix-for-python3.10 branch from 2f5a290 to 6ec6c9f Compare January 16, 2020 05:51
@hugovk
Copy link
Contributor Author

hugovk commented Jan 16, 2020

Updated:

-        if sys.version[:3] != "3.2":
+        if not sys.version_info[:2] == (3, 2):

@nicolaskruchten
Copy link
Contributor

Thanks! I know my feedback is coming across as very-conservative, and I appreciate the followup :)

@@ -227,7 +227,7 @@ def iso_to_plotly_time_string(iso_string):

def template_doc(**names):
def _decorator(func):
if sys.version[:3] != "3.2":
if not sys.version_info[:2] == (3, 2):
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be a [:2] in there? None of the changes in https://github.com/plotly/plotly.py/pull/2078/files include this index...

In fact, I think it might be nice to add this commit to #2078 and close this PR so we can merge it all at once, if you don't mind :)

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 [:2] is needed for == comparisons. It can be used but isn't required for >, >=, <, <= etc.

>>> sys.version_info
sys.version_info(major=3, minor=8, micro=1, releaselevel='final', serial=0)
>>> sys.version_info[:2]
(3, 8)
>>> sys.version_info[:2] == (3, 8)
True
>>> sys.version_info == (3, 8)
False
>>> sys.version_info >= (3, 8)
True
>>> sys.version_info[:2] >= (3, 8)
True
>>> sys.version_info >= (3, 9)
False
>>> sys.version_info[:2] >= (3, 9)
False

Yep, will combine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, I think it might be nice to add this commit to #2078 and close this PR so we can merge it all at once, if you don't mind :)

Added, closing.

@hugovk
Copy link
Contributor Author

hugovk commented Jan 16, 2020

Thanks! I know my feedback is coming across as very-conservative, and I appreciate the followup :)

No problem at all, it's good to be careful with widely used code!

By the way, how about (in another PR) adding python_requires to the setup.py files? That way modern pip (>=9) installs the right version of the package for user's running Python. For example, pip itself has:

    python_requires='>=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*',

Which allows install on Python 2.7 and 3.5+.

https://github.com/pypa/pip/blob/fed360a64e907f3ce8f5d497cc806df2fa9a0799/setup.py#L86

@hugovk hugovk closed this Jan 16, 2020
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.

3 participants