GeezerVison is showing github.com — Allow multiple separators in Stream.readuntil · Issue #81322 · python/cpython
Allow multiple separators in Stream.readuntil #81322Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2019-06-03.09:32:30.680>
labels = ['type-feature', '3.9', 'expert-asyncio']
title = 'Allow multiple separators in Stream.readuntil'
updated_at = <Date 2019-09-26.19:48:53.882>
user = 'https://github.com/bmerry' bugs.python.org fields:
activity = <Date 2019-09-26.19:48:53.882>
actor = 'yselivanov'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2019-06-03.09:32:30.680>
creator = 'bmerry'
dependencies = []
files = []
hgrepos = []
issue_num = 37141
keywords = ['patch']
message_count = 11.0
messages = ['344397', '344398', '344399', '344403', '344404', '344405', '344409', '352089', '352179', '353323', '353335']
nosy_count = 4.0
nosy_names = ['asvetlov', 'yselivanov', 'bmerry', 'Jeffrey.Kintscher']
pr_nums = ['16429']
priority = 'normal'
resolution = 'later'
stage = 'patch review'
status = 'pending'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue37141'
versions = ['Python 3.9'] Activitybmerry commented on Jun 3, 2019 MannequinAuthor More actions Text-based protocols sometimes allow a choice of newline separator - I work with one that allows either \r or \n. Unfortunately that doesn't work with StreamReader.readuntil, which only accepts a single separator, so I've had to do some hacky things to obtain lines without having to
From discussion in bpo-32052, it sounded like extending StreamReader.readuntil to support a tuple of separators would be feasible. asvetlov commented on Jun 3, 2019 Would you make a PR?
I guess to modify Stream.readuntil()
StreamReader is deprecated, I'm not sure if we should add new functionality to this class. bmerry commented on Jun 3, 2019 MannequinAuthor More actions asvetlov commented on Jun 3, 2019 Docs will be updated soon.
The change has landed a week ago, I had no time for docs update before 3.8 beta. Sorry for that.
The idea is: StreamReader and StreamWriter are merged into just Stream, open_connection() is replaced with connect() etc. bmerry commented on Jun 3, 2019 MannequinAuthor More actions Ok. Does the new Stream still have a similar interface for readuntil i.e. is this still a relevant request against the new API? I'm happy to let deprecated APIs stay as-is. asvetlov commented on Jun 3, 2019 Yes, Stream supports all StreamReader and StreamWriter methods bmerry commented on Jun 3, 2019 MannequinAuthor More actions Ok, I've changed the issue title to refer to Stream. Since this would be a new feature, I assume it's off the table for 3.8, but I'll see if I get time to implement a PR in time for 3.9 (and get someone at work to sign off on the contributor agreement, which might be the harder part).
Thanks for the quick and helpful responses. bmerry changed the title [-]Allow multiple separators in StreamReader.readuntil[/-] [+]Allow multiple separators in Stream.readuntil[/+] on Jun 3, 2019bmerry commented on Sep 12, 2019 MannequinAuthor More actions I finally have permission from my employer to sign the contributors agreement, so I'll take a stab at this when I have some free time (unless nobody else gets to it first). asvetlov commented on Sep 12, 2019 gvanrossum commented on Apr 8, 2024 Thanks again!
There's one detail in a different repo that you may want to get ahead of. There's a declaration for this method in typeshed; ideally we should update this conditionally with a check for sys.version_info. bmerry commented on Apr 8, 2024 ContributorAuthor More actions
Thanks again!
There's one detail in a different repo that you may want to get ahead of. There's a declaration for this method in typeshed; ideally we should update this conditionally with a check for sys.version_info.
Thanks for the reminder - I'd meant to ask about typeshed but forgot. I'll sort out a PR there when I get a chance. bmerry commented on Apr 8, 2024 ContributorAuthor More actions Looking at the typeshed entry for readuntil is giving me second thoughts about this PR. In typeshed, it's written like this:
# Can be any buffer that supports len(); consider changing to a Protocol if PEP 688 is accepted
async def readuntil(self, separator: bytes | bytearray | memoryview = b"\n") -> bytes: ...On the other hand, in this PR the new use case is detected like this:
if isinstance(separator, bytes):
separator = [separator]
else:
# Makes sure shortest matches wins, and supports arbitrary iterables
separator = sorted(separator, key=len)So I think this PR will actually break the existing use case of passing a bytearray (or memoryview), because it would be incorrectly interpreted as an iterable of separators rather than a single separator.
@gvanrossum any thoughts on how to better decide whether we've been passed a single separator or an iterable of them? Perhaps instead of isinstance(separator, bytes), one could test hasattr(separator, "__buffer__")isinstance(separator, collections.abc.Buffer)? Or would it be better to introduce a new parameter (or new method) to make the user tell us the intention instead of guessing? bmerry commented on Apr 8, 2024 ContributorAuthor More actions I've opened #117653 to use collections.abc.Buffer instead of bytes to decide the behaviour. gvanrossum commented on Apr 8, 2024
@gvanrossum any thoughts on how to better decide whether we've been passed a single separator or an iterable of them? Perhaps instead of isinstance(separator, bytes), one could test hasattr(separator, "__buffer__")isinstance(separator, collections.abc.Buffer)? Or would it be better to introduce a new parameter (or new method) to make the user tell us the intention instead of guessing?
Well, my intuition would be to only allow tuples -- this follows the lead of e.g. str.startswith():
>>> "abc".startswith("a")
True
>>> "abc".startswith(("a", "b"))
True
>>> "abc".startswith(["a", "b"])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
"abc".startswith(["a", "b"])
~~~~~~~~~~~~~~~~^^^^^^^^^^^^
TypeError: startswith first arg must be str or a tuple of str, not list
>>> bmerry commented on Apr 8, 2024 ContributorAuthor More actions
Well, my intuition would be to only allow tuples -- this follows the lead of e.g. str.startswith():
I can see the appeal in that. It's also consistent with isinstance which takes tuples and not lists.
If someone does pass a list, they're probably going to get a cryptic error message; so it would be nice if we could detect incorrect usage. So maybe if it's not a tuple AND it's not a collections.abc.Buffer, raise a helpful TypeError? gvanrossum commented on Apr 8, 2024
If someone does pass a list, they're probably going to get a cryptic error message; so it would be nice if we could detect incorrect usage. So maybe if it's not a tuple AND it's not a collections.abc.Buffer, raise a helpful TypeError?
Maybe, though I worry about overthinking this. If they pass something incorrect they already get a cryptic error message (e.g. what if they pass a string?). Do we care? Probably not, as long as it doesn't silently do the wrong thing. bmerry commented on Apr 10, 2024 ContributorAuthor More actions I've created a patch to accept only tuples in #117723. bmerry commented on Apr 14, 2024 ContributorAuthor More actions
There's one detail in a different repo that you may want to get ahead of. There's a declaration for this method in typeshed; ideally we should update this conditionally with a check for sys.version_info.
I've opened python/typeshed#11755
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|