Fully annotate ExifRead #9403
Fully annotate ExifRead #9403
Conversation
This comment has been minimized.
This comment has been minimized.
|
For reference, there are potential type problems, which I reported here: ianare/exif-py#172. |
| def find_jpeg_exif(fh, data, fake_exif) -> tuple[Incomplete, Incomplete, Incomplete]: ... | ||
| logger: Logger | ||
|
|
||
| def find_jpeg_exif(fh: Reader, data: bytes, fake_exif: bool) -> tuple[int, bytes, bool]: ... |
There was a problem hiding this comment.
Looks like fake_exif can sometimes be an int: https://github.com/ianare/exif-py/blob/3575cc6d0df9e356edc1cd3d49516d066d1ad465/exifread/jpeg.py#L24
| def find_jpeg_exif(fh: Reader, data: bytes, fake_exif: bool) -> tuple[int, bytes, bool]: ... | |
| def find_jpeg_exif(fh: Reader, data: bytes, fake_exif: int) -> tuple[int, bytes, int]: ... |
There was a problem hiding this comment.
I believe that this is just a leftover from before there was a bool type in Python. We had similar cases in the stdlib.
There was a problem hiding this comment.
I think that's the reason why this is the case, but at least the return type should be int, as I don't think bool is ever returned. It looks like _get_initial_base always returns an int as the second item in the tuple, which means find_jpeg_exif always returns int as the third item in the tuple, even if a bool was passed in.
There was a problem hiding this comment.
The problem with using bool in the return type is that code like the following would cause a type error, although it's working fine at runtime, and is (arguably) correctly annotated:
def foo(b: bool) -> ...:
if b:
...
foo(find_jpeg_exif(...)[2])We've found in the past that just using bool in cases like this works best. The only instance where is fails is if isinstance (or similar) is used at runtime.
| def ord_(dta: str) -> int: ... # type: ignore[misc] | ||
| @overload | ||
| def ord_(dta: _T) -> _T: ... | ||
| def make_string(seq: str | list[int]) -> str: ... |
There was a problem hiding this comment.
I guess this is another one of the str/bytes problems you identified in ianare/exif-py#172.
The code looks like it'll crash if seq isn't a sequence of ints, which implies that the annotation should be bytes | list[int]: https://github.com/ianare/exif-py/blob/3575cc6d0df9e356edc1cd3d49516d066d1ad465/exifread/utils.py#L23.
But in make_string_uc(), it's passed an argument that must be a str: https://github.com/ianare/exif-py/blob/3575cc6d0df9e356edc1cd3d49516d066d1ad465/exifread/utils.py#L47-L53
There was a problem hiding this comment.
Indeed. Therefore, I don't think it particularly matters what we annotate it as at the moment. (Maybe -> NoReturn?) I'm fine with any annotation.
There was a problem hiding this comment.
Yeah. It's a weird one, happy to go with whatever :)
There was a problem hiding this comment.
Let's leave it like it is for lazyness reasons?
Co-authored-by: Alex Waygood <[email protected]>
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
No description provided.