GeezerVison

A few questions about this:

• I've implemented it for both StreamReader and Stream, because it seemed like having two identical (apart from _ensure_can_read) implementations is better for maintainability than two implementations that have drifted out of sync. But if one would prefer not to make any changes to StreamReader I can revert that. I suggest doing review discussion on the Stream implementation.
• Because the existing functionality is implemented using the general case there might be a very small loss of performance. Are there any benchmarks I should run to test that?
• For matches that end in the same position I arbitrarily decided that shortest should win for the purpose of LimitOverrunError, but it was an arbitrary choice. It could just as easily be longest-wins or first-in-list wins.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@1st1 not a problem - definitely worth getting things right before they go into stdlib. Do you think the revert of the new stream API will conflict much with my changes to the old StreamReader class? If not, perhaps the way forward is for me to remove my changes to Stream, address the PR comments on StreamReader and then hopefully it shouldn't need much work to rebase after Stream is removed?

@bmerry I'm not yet sure. I'll be working on a revert later today or tomorrow and will probably know more. I'd keep everything as is as it's not going to be an easy revert anyways.

@1st1 it looks like you've done the Streams reversion now, and I've merged that into my branch. So hopefully this is ready for review now.

@brandtbucher thanks for the suggestions. I've applied all the ones that Github was still showing after the merge from master - let me know if anything got lost along the way.

Still needs updated docs though!

@brandtbucher which docs need updating?

Sorry about the delay @bmerry. The docs I'm referring to are located in Doc/library/asyncio-stream.rst. Specifically, here:

   .. coroutinemethod:: readuntil(separator=b'\\n')

      Read data from the stream until *separator* is found.

      On success, the data and separator will be removed from the
      internal buffer (consumed). Returned data will include the
      separator at the end.

      If the amount of data read exceeds the configured stream limit, a
      :exc:`LimitOverrunError` exception is raised, and the data
      is left in the internal buffer and can be read again.

      If EOF is reached before the complete separator is found,
      an :exc:`IncompleteReadError` exception is raised, and the internal
      buffer is reset.  The :attr:`IncompleteReadError.partial` attribute
      may contain a portion of the separator.

      .. versionadded:: 3.5.2

The signature and description should be updated. Also, something like this should be included below the .. versionadded:: 3.5.2 bit:

      .. versionchanged:: 3.9

         The *separator* parameter may now be an :term:`iterable` of separators.

The docs I'm referring to are located in Doc/library/asyncio-stream.rst

Thanks. I'd assumed the library docs were generated from the docstring, which is why I missed this. I'll update it.

Looks good to me! @1st1?

We will soon start a discussion about the new streaming API for 3.9. I'll update this issue with a link when that happens; please give us some time before making a decision on this one -- Python 3.9 is still relatively far away.

Thanks, I will review in the coming week!