FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

pillow-dependency update by ImportanceOfBeingErnest · Pull Request #11132 · matplotlib/matplotlib · GitHub

Repository navigation

pillow-dependency update - #11132

Merged
timhoffm merged 1 commit into
matplotlib:masterfrom
ImportanceOfBeingErnest:pillow-dependency
Apr 28, 2018
Merged

timhoffm merged 1 commit into
matplotlib:masterfrom
ImportanceOfBeingErnest:pillow-dependency

Conversation

Copy link
Copy Markdown
Member

PR Summary

Document the minimum optional Pillow version.
Fixes #10633.

PR Checklist

  • Code is PEP 8 compliant
  • Documentation is sphinx and numpydoc compliant

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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 _has_pil.

Or are you saying the documentation should pull it from the code?

Copy link
Copy Markdown
Member

Yeah, ideally. Presumably it's also defined in a pip requirements file somewhere too.

Comment thread lib/matplotlib/backend_bases.py Outdated
_has_pil = True
del Image
except ImportError:
except (ImportError, AssertionError):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Instead of catching an AssertionError, I think it would be better to have an if block that checks the version.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, changed that now. Any reason for this? Is it bad style or does it have any side effects?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member Author

Yeah, ideally.

I'd agree, but I don't know how that would be done with Sphinx.

Presumably it's also defined in a pip requirements file somewhere too.

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.

Copy link
Copy Markdown
Member

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.

timhoffm merged commit 9b3ac28 into matplotlib:master Apr 28, 2018
tacaswell added this to the v3.0 milestone Apr 29, 2018
ImportanceOfBeingErnest deleted the pillow-dependency branch May 10, 2018 15:52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

4 participants


Back | FazBrowse Home | New Git URL