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

Import Image from PIL #27940

Closed
wants to merge 1 commit into from
Closed

Conversation

radarhere
Copy link

@radarhere radarhere commented Mar 18, 2024

PR summary

There are places where matplotlib uses, or where the documentation refers to, PIL.Image.open(...).

If users were to look at this, and try and replicate it in their own code, it may not be ideal, since

import PIL
PIL.Image.open(...)

doesn't work.

import PIL.Image
PIL.Image.open(...)

works, but two Pillow core developers (python-pillow/Pillow#6614 (comment) and python-pillow/Pillow#6614 (comment)) have suggested deprecating import PIL.Image.

The ideal code

from PIL import Image
Image.open(...)

is harder for users to figure out without some external knowledge of Pillow.

So this PR attempts to avoid references to PIL.Image, and instead switch to from PIL import Image.

PR checklist

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@tacaswell
Copy link
Member

Thank you for your contributions, however I am pretty negative on these changes.

Reading through those issues I really don't understand why the pillow developers want to deprecate import PIL.Image but leaving that aside I'm pretty sure it is not technically possible as Image.py as https://github.com/python-pillow/Pillow/blob/main/src/PIL/Image.py is a sub-module of the PIL and import package.submodule is Python language thing (as is the non-automatic importing of sub-modules and the global nature of sys.modules). To that end, until pillow actually breaks import PIL.Image I am 👎 on the change in the library source. We have enough "image" like things floating around keep the explicit namespace on PIL.Image is valuable.

I am also 👎🏻 on using the ~ to shorten the names shown for the external cross links to pillow. As noted in one of the issues linked to the linked pillow issue, both PIL.Image (a module) and PIL.Iamge.Image (a class) exist, by using the full names we are very clear which one we talking about when.

Finally, I'm 50/50 on adding the import XXX; to our deprecation notes, but being slightly difficult want it to be import PIL.Image 🤣 .

@timhoffm
Copy link
Member

timhoffm commented Mar 18, 2024

I believe the user confusion mainly stems from the fact that the Image module is capitalized. Thus, by standard naming rules PIL.Image.open() looks like a class Image with a class method open() in the module PIL. With that interpretation, import PIL; PIL.Image.open() would be a reasonable assumption. But that confusion is the responsibility of PIL, and I don’t see why we need to change our code.

@radarhere
Copy link
Author

Ok, thanks for your prompt review.

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.

None yet

3 participants