Continuation of WIP: Type Annotations #2941

wiredfool · 2018-01-08T11:56:23Z

make typecheck, i.e. mypy -2 --ignore-missing-imports src/PIL/Image.py works

…ion.

…ng Filter.

…NG'.

aclark4life · 2018-03-21T14:07:26Z

With all the work done by you and @neiljp (and others?) and at least one other comment indicating this is something people want, I'd say merge-away for April 1 release.

hugovk · 2018-03-24T14:15:37Z

I've not used typing in Python before, but no objections.

Of course, in the future we'll need to make sure new/changed code updates the types. Some instructions in CONTRIBUTING.md would be helpful, and we'll need to make sure contributors' PRs keep things updated, or update them ourselves.

I think it would be useful to check the typing on the CI. I'd suggest a new Travis job that just does the type checking, and no other tests. That way it would run and pass or fail quickly, and wouldn't muddy the waters of whether the actual unit tests pass or fail.

hugovk · 2018-03-24T14:17:43Z

src/PIL/Image.py

@@ -24,6 +24,22 @@
 # See the README file for information on usage and redistribution.
 #

+if False:


Is this if False: going to remain? Does it need to?

A comment to explain why would help.

In this format, these are read by mypy, but don't introduce the dependencies of inclusion at runtime.

it should be this:

import typing if typing.TYPE_CHECKING: ...

hugovk · 2018-03-24T14:18:05Z

src/PIL/Image.py

@@ -65,7 +82,7 @@ def __getattr__(self, id):
                           PILLOW_VERSION))

 except ImportError as v:
-    core = _imaging_not_installed()
+    core = _imaging_not_installed() # type: ignore


pep8: two spaces before inline comment, here and elsewhere

hugovk · 2018-03-24T14:20:09Z

src/PIL/Image.py

@@ -706,21 +743,23 @@ def tobytes(self, encoder_name="raw", *args):

        :param encoder_name: What encoder to use.  The default is to
                             use the standard "raw" encoder.
-        :param args: Extra arguments to the encoder.
+        :param *args: Extra arguments to the encoder. May be one tuple,
+                      or individual arguments


End sentence with . for consistency

hugovk · 2018-03-24T14:24:48Z

src/PIL/Image.py


        if data is None:
            raise ValueError("missing method data")

        im = new(self.mode, size, fillcolor)
        if method == MESH:
            # list of quads
-            for box, quad in data:
+            data_mesh = None # type: List[Tuple[LURD,LURD]]
+            data_mesh = data # type: ignore


What's going on here, setting data_mesh twice?

Yes, as I mentioned above. If you wish to maintain the previous comment of what it "should" be, ideally, then perhaps just put it as the 'post-ignore' comment?

Basically, the first sets the type, and the second actually sets the variable, but we're forcing the typechecker to accept that we know that data is the correct type. This sort of idiom is required(?) when we're unpacking a datastructure that's goat a bunch of type overloads.

neiljp

This is a great followup to our earlier work, and thanks to @wiredfool for persevering! However this is now a very big PR and I partly wonder if it would be good to split it if possible, or squash some commits together. That said, that may be in part due to the size of Image.py.

One thing I am really intrigued by, is the experience of anyone involved of the process - have you identified any latent issues in doing this? is this primarily good for documentation? pushing you towards simpler/better implementations? ...or just an interesting experiment that may as well get merged at this point?

neiljp · 2018-03-24T17:25:36Z

src/PIL/Image.py

@@ -24,6 +24,22 @@
 # See the README file for information on usage and redistribution.
 #

+if False:


In this format, these are read by mypy, but don't introduce the dependencies of inclusion at runtime.

neiljp · 2018-03-24T17:27:26Z

src/PIL/Image.py

@@ -42,6 +58,7 @@ class DecompressionBombError(Exception):
 class _imaging_not_installed(object):
    # module placeholder
    def __getattr__(self, id):
+        # type: (str) -> Any


The return type of this can be NoReturn; this may require the mypy_extensions module based on my experience in Zulip, though it is part of PEP484?

I'm not enclined to worry about the exact type here, as this is just a placeholder object that emits errors when we don't have the c module.

neiljp · 2018-03-24T17:34:30Z

src/PIL/Image.py

        Image.__init__(self)
-        self.tile = []
+        self.tile = []  # type: List[Tuple] ## FIXME Should tile be in __init__ & _new?
+        # FIXME TYPING: ^ Maybe Optional[List[Tuple[Text, LURD, int, Optional[Tuple]]]] ?


It's been a while since I looked at this, but do these FIXME TYPING comments still apply, eg here?

neiljp · 2018-03-24T17:43:37Z

src/PIL/Image.py

@@ -1261,11 +1326,12 @@ def getpalette(self):
            if bytes is str:
                return [i8(c) for c in self.im.getpalette()]
            else:
-                return list(self.im.getpalette())
+                return list(self.im.getpalette()) # type: ignore


When using 'type: ignore', good style appears to be to put a reason afterwards, eg. it could be that types aren't available, are doing dynamic 'patching' which won't ever work, or is eg. a mypy bug! The format used in Zulip is:

some_code # type: ignore # some explanatory text or link

There are several ignores here that may be mypy bugs. I did put those in when I'd spent enough time on it, and it's clear what types we had but not clear how to force mypy to deal with it.

neiljp · 2018-03-24T17:45:05Z

src/PIL/Image.py

            return self.im.histogram(extrema)
        return self.im.histogram()

    def offset(self, xoffset, yoffset=None):
+        # type: (Any, Any) -> Any


I know this has been removed, but this could potentially be eg. (int, Optional[int]) -> NoReturn for completeness?

neiljp · 2018-03-24T17:52:11Z

src/PIL/Image.py

        else:
            # constant alpha
            try:
-                self.im.fillband(band, alpha)
+                alpha_int = None # type: int


Same question here as above - this line is superfluous?

neiljp · 2018-03-24T17:59:55Z

src/PIL/Image.py

@@ -1723,12 +1832,15 @@ def resize(self, size, resample=NEAREST, box=None):
        ):
            raise ValueError("unknown resampling filter")

-        size = tuple(size)
+        size = tuple(size) # type: ignore


Re the following 'Typing note', the parameter could actually be typed as Iterable (or Sequence, if that's plausible too) instead. However that's for backwards compatibility rather than 'what you should pass in', right?

neiljp · 2018-03-24T18:04:11Z

src/PIL/Image.py


        if data is None:
            raise ValueError("missing method data")

        im = new(self.mode, size, fillcolor)
        if method == MESH:
            # list of quads
-            for box, quad in data:
+            data_mesh = None # type: List[Tuple[LURD,LURD]]
+            data_mesh = data # type: ignore


Yes, as I mentioned above. If you wish to maintain the previous comment of what it "should" be, ideally, then perhaps just put it as the 'post-ignore' comment?

neiljp · 2018-03-24T18:07:48Z

src/PIL/Image.py

-    pass
-
+    def point(self, s):
+        # type (T) -> T


What is T here? This doesn't look familiar from when I was working on this. My instinct is a TypeVar, but T is not very descriptive?

neiljp · 2018-03-24T18:12:39Z

src/PIL/Image.py

+    # and obj_bytes, if it exists, is a bytes exporting a buffer interface.
+    # Unfortunately at this point, there's no protocol for the buffer interface
+    # and due to it's c-level implementation, I don't see how to make one.
+    return frombuffer(mode, size, obj_bytes or obj, "raw", rawmode, 0, 1) # type: ignore


Maybe file an issue in typeshed re this, if there's not one already? If typeshed is the best place - maybe even python/typing? (for more general language/typing queries)

homm · 2018-04-01T11:37:36Z

.gitignore

@@ -78,3 +78,4 @@ Tests/images/README.md
 Tests/images/msp
 Tests/images/picins
 Tests/images/sunraster
+/.mypy_cache/


This section of .gitignore is for "Extra test images installed from pillow-depends/test_images". Better place is "Unit test / coverage reports"

homm · 2018-04-01T11:41:17Z

src/PIL/ImageFilter.py

-    pass
-
+    def filter(self, image):
+        return image.im


The image argument is already an image core object.

wiredfool · 2018-04-01T16:37:21Z

One thing I am really intrigued by, is the experience of anyone involved of the process - have you identified any latent issues in doing this? is this primarily good for documentation? pushing you towards simpler/better implementations? ...or just an interesting experiment that may as well get merged at this point?

I wouldn't say that it's pushing things to be a simpler implementation, because in many cases we're stuck with the interface we have, and that interface was not designed to be type safe. There are a few idioms, like a single type static assignment that work, and are at least one way to do things. But they aren't simple, they don't seem pythonic, and they obscure the flow of what's going on.

I could see where this might guide IDEs to make better suggestions. I can't recall if this process actually uncovered any bugs in Pillow, but my feeling at the end of it was more that this was an interesting exercise than a leap forward in code quality.

Stadly · 2020-10-03T10:11:20Z

What's the status on this? Type annotations for Pillow would be great!

hugovk · 2023-10-16T20:14:30Z

This has conflicts and a lot has changed in typing since this was opened. Let's close in favour of a more incremental approach along the lines of #2625 (comment). Thanks anyway for all your work here.

neiljp added 30 commits January 3, 2018 10:31

Type annotations: Image.py.

97c2312

Type annotations: Switch to type alias for Mode & add self.im annotat…

43b704b

…ion.

Type annotations: Adjust Filter import.

bd4e593

Type annotations: Adjust __repr__ return & ignore ne/eq.

cd26623

Type annotations: Add stubs for _imaging; aliases into aliases.py.

0b00559

Type annotations: Mark Image.draft to accept (Mode, Size).

9625b79

Type annotations: Add & use Extrema type alias.

850e28a

Type annotations: Improve Image.getproject().

5979e0e

Type annotations: Improve imports in Image.py.

cbe9522

Type annotations: Explicitly mark some Image data types.

2c76f3b

Type annotations: Correct Image/ImagingCore annotations.

898c83d

Type annotations: Annotate exclusive_fp.

04f7ac3

Type annotations: Annotate self.tile & add FIXME.

363e409

Type annotations: Add Union & Any to aliases.py.

4c45a31

Type annotations: Amend eq/ne to be (object)->bool.

2d0fb6b

Type annotations: Amend __setstate__ to accept Tuple.

cb535be

Type annotations: Enable Image.filter to also accept Callable returni…

0541e23

…ng Filter.

Type annotations: Amend Image.point parameter types.

2110a7f

Type annotations: Correct pixel_access & specify self.readonly.

cf043ed

Type annotations: _imaging.pyi: Significantly improve stubs.

48e183e

Type annotations: Allow Image.putpalette to accept ImagePalette.

2ab28d5

Type annotations: Clarify return type of Image.split.

910236f

Type annotations: aliases.py: Amend Extrema to use float & int Tuples.

5515520

Type annotations: Amend Image.getdata to return ImagingCore.

636e740

Type annotations: Image.rotate bool->int for a parameter.

10e2908

Type annotations: Switch open/save BytesIO->BinaryIO.

ab40a28

Type annotations: Label all obvious issues and label with 'FIXME TYPI…

1af8658

…NG'.

Type annotations: aliases.py: Add Color type-alias (no Text).

1c0acf8

Type annotations: Image.py: Improve many annotations.

8db8e32

Type annotations: _imaging.pyi: Many improvements.

72393d7

wiredfool added Needs Documentation Needs Release Notes labels Mar 21, 2018

wiredfool added this to the 5.1.0 milestone Mar 21, 2018

hugovk mentioned this pull request Mar 24, 2018

[WIP]: Type annotations for Image.py #2687

Closed

hugovk reviewed Mar 24, 2018

View reviewed changes

neiljp reviewed Mar 24, 2018

View reviewed changes

homm reviewed Apr 1, 2018

View reviewed changes

src/PIL/ImageFilter.py

pass

def filter(self, image):

return image.im

Copy link

Member

homm Apr 1, 2018

Choose a reason for hiding this comment

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

The image argument is already an image core object.

radarhere modified the milestones: 5.1.0, 5.2.0 Apr 10, 2018

radarhere added the Needs Rebase label Jul 2, 2018

radarhere removed this from the 5.2.0 milestone Jul 2, 2018

Merge branch 'master' into pr_2687

45752d2

radarhere removed the Needs Rebase label Feb 15, 2019

radarhere and others added 2 commits February 15, 2019 23:25

Lint fixes

14e7f8a

Merge branch 'master' into pr_2687

fdebb6f

radarhere added the Needs Rebase label Mar 11, 2019

python-pillow deleted a comment from codecov bot Aug 27, 2019

hugovk force-pushed the master branch from 747e44b to 9e3ad5e Compare December 18, 2020 20:12

wiredfool mentioned this pull request May 21, 2021

Support for typing? #2625

Closed

radarhere mentioned this pull request Sep 30, 2022

Don't reassign crc on ChunkStream close #6627

Merged

hugovk closed this Oct 16, 2023

hugovk added the Type hints label Dec 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continuation of WIP: Type Annotations #2941

Continuation of WIP: Type Annotations #2941

wiredfool commented Jan 8, 2018

aclark4life commented Mar 21, 2018

hugovk commented Mar 24, 2018

hugovk Mar 24, 2018

neiljp Mar 24, 2018

and-semakin Nov 5, 2019

hugovk Mar 24, 2018

hugovk Mar 24, 2018

hugovk Mar 24, 2018

neiljp Mar 24, 2018

wiredfool Apr 1, 2018

neiljp left a comment

neiljp Mar 24, 2018

neiljp Mar 24, 2018

wiredfool Apr 1, 2018

neiljp Mar 24, 2018

neiljp Mar 24, 2018

wiredfool Apr 1, 2018

neiljp Mar 24, 2018

neiljp Mar 24, 2018

neiljp Mar 24, 2018

neiljp Mar 24, 2018

neiljp Mar 24, 2018

neiljp Mar 24, 2018

homm Apr 1, 2018

homm Apr 1, 2018

wiredfool commented Apr 1, 2018

Stadly commented Oct 3, 2020

hugovk commented Oct 16, 2023

Continuation of WIP: Type Annotations #2941

Continuation of WIP: Type Annotations #2941

Conversation

wiredfool commented Jan 8, 2018

aclark4life commented Mar 21, 2018

hugovk commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiredfool commented Apr 1, 2018

Stadly commented Oct 3, 2020

hugovk commented Oct 16, 2023