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

Remove some references to Py2. #15838

Merged
merged 1 commit into from Dec 5, 2019
Merged

Remove some references to Py2. #15838

merged 1 commit into from Dec 5, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Dec 5, 2019

The checks in conf.py and matplotlib/__init__.py are not useful
because the files are in fact not Py2-compatible (this has been the case
for __init__.py for a while...) due to f-strings/nonlocal, so someone
trying to run the file on Py2 will get a SyntaxError instead of the
error message.

In image.py the cast to int remains necessary even on Py3 because
round(np.float) returns a np.float, not a np.int.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

The checks in `conf.py` and `matplotlib/__init__.py` are not useful
because the files are in fact not Py2-compatible (this has been the case
for `__init__.py` for a while...) due to f-strings/nonlocal, so someone
trying to run the file on Py2 will get a SyntaxError instead of the
error message.

In image.py the cast to int remains necessary even on Py3 because
round(np.float) returns a np.float, not a np.int.
@tacaswell tacaswell added this to the v3.3.0 milestone Dec 5, 2019
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Would be better if we could have the warning, but it is not worth making these files py2 again to get it.

@@ -1167,7 +1167,6 @@ def make_image(self, renderer, magnification=1.0, unsampled=False):
l, b, r, t = self.axes.bbox.extents
width = (round(r) + 0.5) - (round(l) - 0.5)
height = (round(t) + 0.5) - (round(b) - 0.5)
# The extra cast-to-int is only needed for python2
width = int(round(width * magnification))
height = int(round(height * magnification))
Copy link
Member

Choose a reason for hiding this comment

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

Should this PR also remove the cast to int indicated by the comment as only beeing needed in py2?

Copy link
Member

Choose a reason for hiding this comment

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

The comment was wrong, we need to cast to int in both cases as round returns a float and down-stream code requires an int.

@timhoffm
Copy link
Member

timhoffm commented Dec 5, 2019

Would be better if we could have the warning, but it is not worth making these files py2 again to get it.

The codebase is Py3 anyway nobody should expect it to be able to run it on Py2.

@timhoffm timhoffm merged commit c42fdda into matplotlib:master Dec 5, 2019
@anntzer anntzer deleted the py2 branch December 5, 2019 19:17
@tacaswell
Copy link
Member

It is belt-and-suspenders against clever users that find someway to run into this. The one that came up most recently was someone had py2 installed, cloned the repo and tried to build the docs against their installed version of Matplotlib.

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.

None yet

4 participants