Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
gh-127647: Add typing.Reader and Writer protocols #127648
gh-127647: Add typing.Reader and Writer protocols #127648
Changes from 29 commits
b45fec0
1525e05
7867ec1
6a22a02
5d632a3
4d50c2e
1e1ea41
56a38a0
f2c331b
6764b6a
022acaa
b86073d
65eb040
2b9159d
1f42b21
0325f5a
632511a
3b384f9
5bdb4cc
35dcaf4
5584a57
af81301
5a8b915
b1593fa
577b893
cedfa42
a0b9e47
53a2250
03aa3a2
ca72c19
3b5975e
96080fe
3723370
76003a8
43e23f0
c644770
bfab2fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if this has been discussed before, but I'm unsure on the runtime use of
size=...
(I didn't notice this earlier in my documentation review, sorry).Almost every other
read(size)
method I can find has a default of eitherNone
or-1
. I also can't find another method in the stdlib with a default of...
(outside of the recently-added protocols inwsgiref.types
).Would it be better to have
size=-1
, to indicate that the method takes anint
? I'm not sure how much we want typeshed-like practices to leak into the standard library.A
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mandating defaults is not really something you can do in a protocol. I also wouldn't want to mandate that implementors have to use a default of
-1
, because – as you said – some implementations useNone
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but this isn't a protocol -- it's an ABC, which do have defaults -- see e.g.
collections.abc.Generator
. All of theread()
methods inio
are documented as havingread(size=-1, /)
, and given these ABCs are going intoio
, I think we should be consistent with that interface, or have good reason to diverge from it (& document why).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's supposed to be a protocol, not an ABC. (Notwithstanding the fact that all protocols are ABCs.) It's just a protocol in the implementation for performance reasons. And using
-1
as a default would give users the very wrong impression that they can useread(-1)
when that may or may not actually be supported.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the documentation of
io.RawIOBase.read()
:I would expect the
io.Reader.read()
ABC/protocol to have this same guarantee, for a 'properly' implementedread()
method (according to theio
expecations). In the proposed documentation, we say:This forbids
None
(good!), but is silent on what happens should size be omitted. I still think we should use-1
instead of...
, but at the very least we should include in the documentation the contract for what happens whenread()
is called with no arguments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree.
-1
is the default for some implementations, but the protocol should not make any default mandatory.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure I follow, though -- currently the ABC/protocol has the mandatory default of
size=...
, which is always invalid and not the correct type.-1
is valid for all interfaces specified by theio
documentation, which is where this new type is being added.I ran the following with
mypy --strict
and it passed, so I don't think type checkers care about default values (and as discussed, the type forbids using non-integer types):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Sebastian here; we should use
...
because the protocol need not mandate any particular default.The protocol should also match other file-like classes defined elsewhere in the stdlib or even in third-party libraries. When defining a protocol it's often useful to be permissive, so that all objects that are intended to match the protocol actually match it.