-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
PKG: Declare Pillow as optional dependency #1392
Conversation
9998a59
to
57e690b
Compare
@MasterOdin What do you think about this? I think (hope) that we don't get more dependencies, but with I don't see a way around the PIL / cryptodome dependencies if we want to have those features. And I think those features add enough value to justify an optional dependency. I am uncertain about the name "full". Maybe "all" would be better? |
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.
Should include a link on https://github.com/py-pdf/PyPDF2/blob/main/docs/user/extract-images.md back to the installation page referencing that need to install [image]
.
May also want to update https://pypdf2.readthedocs.io/en/latest/modules/PageObject.html#PyPDF2._page.PageObject.images to include that reference as well?
Codecov ReportBase: 94.19% // Head: 94.19% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1392 +/- ##
=======================================
Coverage 94.19% 94.19%
=======================================
Files 28 28
Lines 5082 5085 +3
Branches 968 968
=======================================
+ Hits 4787 4790 +3
Misses 176 176
Partials 119 119
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Additionally, I think that a try/except should be added around: where we do something like: try:
from PIL import Image
except ImportError:
raise DependencyError("pillow is required to do image extraction") This is similar to what we do with We definitely do need to add something, as shown in the case for the user in #1390, they'll get an error about |
raise ImportError( | ||
"pillow is required to do image extraction. " | ||
"It can be installed via 'pip install PyPDF2[image]'" | ||
) |
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.
If wanted a test here, could add a test with monkeypatch.setitem
modifying sys.modules
to remove it from the environment. See https://stackoverflow.com/a/28361013/4616655 for an example.
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.
That's an awesome idea! Thanks for sharing it - I've written the test!
Closes #1390