-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Added type hints to Image.__init__() #8279
Conversation
|
I'm a tiny bit worried about the cost of these assertions within the core library. I think practically no one* runs Python with *: entirely anecdotal |
Some discussion on https://discuss.python.org/t/does-anyone-use-o-or-oo-or-pythonoptimize-1-in-their-deployments/37671 Please could you run some benchmarks to evaluate the cost? |
There's a variety of changes here, but running
without this change, I get
With this change, I get
|
I think that's close enough not to worry about. There's some variation as well. Running twice: without this change, I get:
with this change, I get:
And similar results across 3.12. Without:
With:
@akx Any further concerns? |
Now that I think of it, can Would all of this be simpler if we went for practicality and not purity, and said
or alternatively left
as an annotation? I think #7461 is related here. |
There are other places where it is set to Pillow/src/PIL/PngImagePlugin.py Line 871 in 11b4df3
Pillow/src/PIL/GifImagePlugin.py Line 158 in 11b4df3
Pillow/src/PIL/GifImagePlugin.py Line 437 in 11b4df3
The current code does also check that it isn't Line 890 in 11b4df3
Line 907 in 11b4df3
|
If it's only about 5 places, would it make sense to consider whether those places need to do that? They could |
c953562
to
4d0e8e7
Compare
This isn't just about the edge case of calling >>> from PIL import Image
>>> im = Image.open("Tests/images/hopper.png")
>>> im.im is None
True
>>> im.load()
<PixelAccess object at 0x105d86510>
>>> im.im is None
False
If code simplicity is the concern rather than speed, how about this idea as a slightly smaller change? I've pushed a commit so that using |
@property | ||
def im(self) -> core.ImagingCore: | ||
if isinstance(self._im, DeferredError): | ||
raise self._im.ex | ||
assert self._im is not None | ||
return self._im |
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.
Couldn't self._im
still be None
here?
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 it is, the assert
will stop the code before the return
statement is reached.
Alternative to #8275
Once the
None
return type is added toImage.__init__()
, a variety of other changes are needed.Pillow/src/PIL/Image.py
Lines 535 to 544 in d8447de