Skip to content
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 #40

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Added type hints #40

merged 3 commits into from
Jun 25, 2024

Conversation

radarhere
Copy link

@radarhere radarhere commented Jun 22, 2024

Suggestions for python-pillow#8032

  • Added some type hints to tests to go with the ImageGrab changes
  • Removed a cast in Image.py that isn't needed
  • Used a new variable in ImageGrab instead of casting, which I think is simpler

src/PIL/Image.py Outdated
cast(Tuple[int, int, int], trns), # trns was converted to RGB
new_im,
)
new_im.info["transparency"] = new_im.palette.getcolor(trns, new_im)
Copy link
Owner

@nulano nulano Jun 24, 2024

Choose a reason for hiding this comment

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

I'm not too sure about this change.
Although mypy does not complain either way, I can see a warning in PyCharm without the cast.
Running mypy with --strict prints a warning that could explain why it (incorrectly) does not error without the cast:

src\PIL\Image.py:1158: error: Item "None" of "Optional[Any]" has no attribute
"getcolor"  [union-attr]
    ...                   new_im.info["transparency"] = new_im.palette.getcol...
                                                        ^~~~~~~~~~~~~~~~~~~~~~~

I suspect that the warning is only skipped because mypy doesn't realize that new_im.palette is an ImagePalette because it is set within an if block (unlike the cast above which is not in a nested if block).

Edit: clarify wording

Copy link
Author

Choose a reason for hiding this comment

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

I've created python-pillow#8167, where I give up on the idea of requiring getcolor() to have a length of exactly 3 or 4. If that is merged, then my next suggestion would be radarhere@dfc1b0f

But I am keen to get python-pillow#8032 in for this release, so at any stage if you say 'Hey, let's merge it as it is and debate this later', we absolutely can.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I'd prefer to discuss this commit separately then.

Could you rebase to remove the commit so I can merge the rest of this PR?
And put it in a separate PR to the main repo once python-pillow#8032 and python-pillow#8167 are merged?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I've removed the commit.

Copy link
Author

Choose a reason for hiding this comment

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

Both the PRs have been merged, so I've created python-pillow#8169

Co-authored-by: Ondrej Baranovič <ondreko.tiba@gmail.com>
@nulano nulano merged commit ab18395 into nulano:type_hints Jun 25, 2024
50 of 51 checks passed
@nulano
Copy link
Owner

nulano commented Jun 25, 2024

Thanks

@radarhere radarhere deleted the type_hints branch June 25, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants