-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
pillow-dependency update #11132
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
pillow-dependency update #11132
Conversation
4ffdb54 to
3679467
Compare
|
It really annoys me that the string "3.4" is hardcoded in several places in the code, instead of being in one place that it is pulled from, but I guess there's no easy way to do that? |
|
This PR changes the code to have that number appear only once in the code, namely in backend_bases.py. Any descendent backend can import Or are you saying the documentation should pull it from the code? |
|
Yeah, ideally. Presumably it's also defined in a pip requirements file somewhere too. |
lib/matplotlib/backend_bases.py
Outdated
| _has_pil = True | ||
| del Image | ||
| except ImportError: | ||
| except (ImportError, AssertionError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of catching an AssertionError, I think it would be better to have an if block that checks the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, changed that now. Any reason for this? Is it bad style or does it have any side effects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just more specific than catching a general error, that might be raised by other bits of code that aren't the specific assertion line you added.
I'd agree, but I don't know how that would be done with Sphinx.
I don't think so because Pillow is not actually a requirement, just an optional thing, which, if present, allows to have further functionality included. |
3679467 to
5e858a6
Compare
|
Don‘t know either how it could be done with Sphinx. But since we‘ll rarely change the version dependency, it’s ok to hard code in the docs. |
PR Summary
Document the minimum optional Pillow version.
Fixes #10633.
PR Checklist