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 4 #2078

Merged
merged 3 commits into from
Jan 22, 2020
Merged

Fix for Python 4 #2078

merged 3 commits into from
Jan 22, 2020

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Jan 15, 2020

Python 4 is some way off, but it may be a good idea to clean some of these up, similar to #2076.


This comparison is True for Python 3.3 - 3.99, but is False for Python 4 and would run the Python 2 - 3.2 code:

if sys.version_info.major == 3 and sys.version_info.minor >= 3:	
    from unittest.mock import patch
else:
    from mock import patch	

Because Python 3.3 is the minimum Python 3.x supported, it can be simplified:

-if sys.version_info.major == 3 and sys.version_info.minor >= 3:	
+if sys.version_info.major >= 3:	

Another couple of cases which fail on Python 4.0, here we need to compare the version_info tuple and avoid comparing the sys.version_info.minor integer:

-if sys.version_info.major == 3 and sys.version_info.minor >= 4:
+if sys.version_info >= (3, 4):
-if sys.version_info.major != 3 or sys.version_info.minor < 6:
+if sys.version_info < (3, 6):

Finally, there's some code which uses six.PY3, a bit like:

if six.PY3:
    print("Python 3+ code")
else:
    print "Python 2 code"

Where in six.py:

PY3 = sys.version_info[0] == 3

When run on Python 4, this will run the Python 2 code!

Instead, use six.PY2.


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

@nicolaskruchten
Copy link
Contributor

Thanks for this! I don't understand the following reasoning though:

Because Python 3.3 is the minimum Python 3.x supported, it can be simplified:

-if sys.version_info.major == 3 and sys.version_info.minor >= 3:
+if sys.version_info.major >= 3:

wouldn't this be a change in behaviour for 3.2?

@hugovk
Copy link
Contributor Author

hugovk commented Jan 15, 2020

Yes, it would be a change in behaviour for 3.2.

Does plotly.py still support 3.2?

The classifiers at https://pypi.org/project/plotly/ suggest 2.7 and 3.3 - 3.7.

@nicolaskruchten
Copy link
Contributor

No, but I'd rather these changes be pure refactorings that don't change behaviour if possible, to avoid unintended side-effects :)

@hugovk
Copy link
Contributor Author

hugovk commented Jan 15, 2020

Sure thing, will update tomorrow!

@nicolaskruchten
Copy link
Contributor

Thanks very much for bringing this up and fixing it for us :)

@hugovk
Copy link
Contributor Author

hugovk commented Jan 16, 2020

You're welcome, and updated:

-if sys.version_info.major == 3 and sys.version_info.minor >= 3:
+if sys.version_info >= (3, 3):

@nicolaskruchten
Copy link
Contributor

LGTM... @jonmmease @emmanuelle thoughts?

@hugovk
Copy link
Contributor Author

hugovk commented Jan 16, 2020

As suggested in #2076 (comment), I've combined PR #2076 with this one.

@nicolaskruchten
Copy link
Contributor

OK nice so this basically mostly impacts tests, and all the tests are passing! We'll merge this in before the 4.5 release next week unless anyone brings up any objections by then. Thanks again!

@nicolaskruchten nicolaskruchten added this to the v4.5.0 milestone Jan 16, 2020
@emmanuelle emmanuelle merged commit 7f6713f into plotly:master Jan 22, 2020
@emmanuelle
Copy link
Contributor

Thank you very much!

@hugovk hugovk deleted the fix-for-python4 branch January 22, 2020 14:11
@hugovk
Copy link
Contributor Author

hugovk commented Jan 22, 2020

You're welcome!

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