Skip to content

typing: Add peek method to BinaryIO (#3951) #3957

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

Closed
wants to merge 4 commits into from
Closed

typing: Add peek method to BinaryIO (#3951) #3957

wants to merge 4 commits into from

Conversation

vegarsti
Copy link
Contributor

@vegarsti vegarsti commented Apr 30, 2020

Issue.

In the below example, f is of type BinaryIO and has method peek (docs), but mypy doesn't know this.

with open("some-file", 'rb') as f:
    f.peek()

The size parameter to peek is optional. Is size: int = ... the correct way to indicate that? That's what I saw other places in the code base.

@vegarsti
Copy link
Contributor Author

vegarsti commented Apr 30, 2020

I see that it's failing in CI, I'll try to find out why. EDIT: Fixed.

@srittau
Copy link
Collaborator

srittau commented Apr 30, 2020

Thank you for the PR. IO types are currently a bit of a mess. The long term plan is to have concrete classes returned from functions. Currently, open() returns IO[Any] in Python 3 and BinaryIO in Python 2.

The implementation of typing.BinaryIO does not have a peek() method, so we should not add it to our stubs. But of course since the object returned by open() does have peek() we should represent this in the stubs somehow. Maybe it's time to create a custom file type for open(), deriving from IO/BinaryIO in typeshed. @JelleZijlstra what do you think?

@vegarsti
Copy link
Contributor Author

Ah, I see! Makes sense not to add this, then. Let me know how I can help with this!

@vegarsti
Copy link
Contributor Author

@JelleZijlstra a friendly ping

@JelleZijlstra
Copy link
Member

I guess we can try returning custom types from open(). I'm worried that something will break just because it is so widely used, but we'll see.

@srittau
Copy link
Collaborator

srittau commented May 19, 2020

Yes, something breaking is a real danger, we certainly need to test this change.

@JelleZijlstra
Copy link
Member

Let's close this because BinaryIO.peek should not exist, and discuss next steps in the issue.

@vegarsti vegarsti deleted the add-peek-to-binaryio branch May 29, 2020 13:11
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.

3 participants