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

Type annotate as IO[bytes], not BinaryIO #215

Merged
merged 1 commit into from
Nov 9, 2022
Merged

Type annotate as IO[bytes], not BinaryIO #215

merged 1 commit into from
Nov 9, 2022

Conversation

hukkin
Copy link
Owner

@hukkin hukkin commented Nov 9, 2022

Fixes #214

I'm currently not creating a SupportsRead because Protocols only appear in Python 3.8 and we need to support 3.7 (and I don't want a typing-extensions dependency).

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Great! This should fix the user's issue.

Note that you don't necessarily have to have a runtime dependency on typing_extensions. Type checkers all special case typing_extensions, so they'll still recognise the following even if typing_extensions is not installed:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from typing_extensions import Protocol

    class SupportsReadBytes(Protocol):
        def read(self) -> bytes: ...
else:
    SupportsReadBytes = object

That said, it is pretty gross and it looks like this hasn't come up much, so fine to keep PR as is.

@hukkin hukkin merged commit 5646e69 into master Nov 9, 2022
@hukkin hukkin deleted the no-binaryio branch November 9, 2022 23:01
@hukkin
Copy link
Owner Author

hukkin commented Nov 9, 2022

Thanks Shantanu for your insight on this one!

Yeah, I agree it's a bit gross so let's try to avoid it until a user complains. Hopefully that never happens, as more and more users move to new Python versions and tomllib 😃

@mgorny
Copy link

mgorny commented Nov 10, 2022

That said, i wish some basic protocols like SupportsReadBytes were part of stdlib. It feels like it's going to end up being repeated a lot.

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.

Type stub incompatiblity with tarfile.ExFileObject (i.e. tarfile.TarFile.extractfile() result)
3 participants