Skip to content

Replace an instance of IO with a protocol #5471

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

Merged
merged 3 commits into from
May 19, 2021
Merged

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented May 16, 2021

No description provided.

@Akuli
Copy link
Collaborator

Akuli commented May 16, 2021

Can we assume that type checkers realize how bool is Union[Literal[True], Literal[False]], and therefore close_fileobj=some_bool works?

@srittau
Copy link
Collaborator Author

srittau commented May 16, 2021

Can we assume that type checkers realize how bool is Union[Literal[True], Literal[False]], and therefore close_fileobj=some_bool works?

It seems that mypy still can't. Changing the last overload to bool. That's potentially unsafe (as close is missing), but I think that's a reasonable tradeoff, considering this class is not really meant to be instantiated by the user anyway.

mode: str,
zipinfo: ZipInfo,
pwd: Optional[bytes] = ...,
close_fileobj: bool = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this works as intended. If fileobj is a _ZipStream but not a _ClosableZipStream, and you pass close_fileobj=True, it will match this overload instead of erroring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See above: I think this is a reasonable tradeoff, considering our "prefer false negatives over false positives" policy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we might as well get rid of _ClosableZipStream altogether, because distinguishing between closable and non-closable zip streams doesn't affect whether the code type-checks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But having it separate has the advantage that the following fails:

ZipExtFile(my_non_closable_stream, close_fileobj=True)

The only thing that can't be checked properly is the following:

close: bool = True
ZipExtFile(my_non_closable_stream, close_fileobj=close)

Copy link
Collaborator

@Akuli Akuli May 16, 2021

Choose a reason for hiding this comment

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

I don't think that fails. It doesn't match the first two overloads, but it does match the third overload which just requires any stream, and bool for close_fileobj. (Note that True converts to bool.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. At this point I think that maybe the first solution was the best, and until python/mypy#6113 is fixed, people just use # type: ignore?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we prefer false negatives, it might be better to just never require a .close() method.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit de0c62f into python:master May 19, 2021
@srittau srittau deleted the zip-io branch May 19, 2021 07:23
@Akuli
Copy link
Collaborator

Akuli commented May 19, 2021

So now there is an unnecessary class _ClosableZipStream on master?

@srittau
Copy link
Collaborator Author

srittau commented May 19, 2021

I opened #5504 to improve this situation.

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