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

Add type hints to _binary #7659

Merged
merged 2 commits into from
Dec 31, 2023
Merged

Add type hints to _binary #7659

merged 2 commits into from
Dec 31, 2023

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Dec 31, 2023

Helps #2625.

There are two tricky parts in adding type hints to PIL._binary:

The i16* and i32* give a "Returning Any from function declared to return int" in --strict mode due to the use of struct.unpack_from. This can be properly silenced with #type: ignore[no-any-return], but that gives a "Unused type ignore comment" when running without --strict, so I have left it out for now.

The i8 function distinguishes between int and bytes inputs in a way that mypy doesn't recognize. Since i8 is called with an int input only from IptcImagePlugin, I have removed those calls there and removed int support as input to i8.

Although the values in self.info[(3, 60)] and self.info[(3, 65)] can be lists if the tags are duplicated, these tags are marked as "non-repeatable" in the specification (pg. 13-14). The specification recommends using the first entry if the tags are duplicated (sec. 1.5-d), but this recommendation was already ignored in IptcImagePlugin and I was unable to find any images to test this with.

In doing so, I found that IptcImagePlugin.dump, IptcImagePlugin.i, and IptcImagePlugin.PAD are simple helpers that should probably be deprecated (edit: #7664).

@hugovk
Copy link
Member

hugovk commented Dec 31, 2023

This looks fine, please can you split this into two PRs, one to deprecate and the other to add type hints?

I think it will be clearer to keep them separate.

If you want, it's okay to leave this PR as-is, and create another one that is the first step, and we can merge that one first.

@nulano nulano marked this pull request as draft December 31, 2023 14:22
@nulano nulano marked this pull request as ready for review December 31, 2023 14:23
@nulano nulano changed the title Add type hints to _binary and deprecate IptcImagePlugin helpers Add type hints to _binary Dec 31, 2023
@nulano
Copy link
Contributor Author

nulano commented Dec 31, 2023

OK, I've removed the deprecation commits and created #7664 for that.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thanks!

@hugovk hugovk removed the Deprecation Feature that will be removed in the future label Dec 31, 2023
@hugovk hugovk merged commit 6282caf into python-pillow:main Dec 31, 2023
56 checks passed
@nulano nulano deleted the types-binary branch December 31, 2023 16:31
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.

None yet

2 participants