Skip to content

io.IncrementalNewlineDecoder is not a subclass of codecs.IncrementalDecoder #11135

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 1 commit into from

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Dec 10, 2023

The documentation says that this class "inherits codecs.IncrementalDecoder", but it's not true for the C implementation.

The python implementation, _pyio.IncrementalNewlineDecoder, does inherit from codecs.IncrementalDecoder, but that's not the implementation that you get with from io import IncrementalNewlineDecoder.

cpython has a ticket for this issue here: python/cpython#75903

In the meantime, I think this is more correct.

Related to #3968

…ecoder

The documentation says that this class "inherits codecs.IncrementalDecoder",
but it's not true for the C implementation.

The python implementation, _pyio.IncrementalNewlineDecoder, *does*
inherit from codecs.IncrementalDecoder, but that's not the implementation
that you get with `from io import IncrementalNewlineDecoder`.

cpython has a ticket for this issue here: python/cpython#75903

In the meantime, I think this is more correct.

Related to python#3968
Copy link
Contributor

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

@tungol
Copy link
Contributor Author

tungol commented Dec 10, 2023

A little more investigation shows that the situation with io.BufferedRandom and io.StringIO is very similar. Typeshed is currently following inheritance given in the documentation and/or python implementation, not the C implementation. That said, those two are slightly muddier, so I'll wait and see what kind of reception this MR gets before doing anything about those two.

@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 10, 2023

We should be careful here. I imagine the intention might be for these io classes to be "abstract" subclasses of the superclasses given in the docs (they implement the interface of the superclass even if they don't actually have the superclass in their mro at runtime). It might be a good idea to keep the fake inheritance here, for the same reason that we should keep pretending that builtins.list inherits directly from typing.MutableSequence

@tungol
Copy link
Contributor Author

tungol commented Dec 10, 2023

That makes sense. It seems like the main reason it isn't a subclass right now is that it's difficult to subclass a python implementation from a C implementation. I think you're right that the intention is there. I'll close this out for now.

@tungol tungol closed this Dec 10, 2023
@JelleZijlstra
Copy link
Member

We should be careful here. I imagine the intention might be for these io classes to be "abstract" subclasses of the superclasses given in the docs (they implement the interface of the superclass even if they don't actually have the superclass in their mro at runtime). It might be a good idea to keep the fake inheritance here, for the same reason that we should keep pretending that builtins.list inherits directly from typing.MutableSequence

Not sure I agree. It's important to get inheritance right so that type checkers can treat isinstance() calls on these classes correctly. isinstance([], collections.abc.MutableSequence) is true at runtime (because of ABC registration), but as I understand that's not the case here.

@AlexWaygood
Copy link
Member

I didn't actually mean my comment as a "This is wrong, close this now" kind of thing, more of a "Beware of Chesterton's fence" kind of thing. Feel free to reopen if I'm incorrect here, I haven't looked too deeply at this yet!

@tungol
Copy link
Contributor Author

tungol commented Dec 11, 2023

I didn't take your comment that way, for what it's worth. I closed this ticket because there's a bunch of other inheritance issues I'm working through, too. Once I've cleared out more of the easier cases, I'll probably revisit this.

@tungol tungol deleted the IncrementalNewlineDecoder branch October 5, 2024 23:50
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