Skip to content
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-100226: Clarify StreamReader.read behavior #101807

Merged
merged 5 commits into from
Feb 17, 2023

Conversation

jgosmann
Copy link
Contributor

@jgosmann jgosmann commented Feb 10, 2023

This clarifies the StreamReader.read behavior in the public documentation; mostly copying from the more precise docstring of the function.

While we're at it, also fix some minor grammar in the docstring itself.

Addresses #100226.

Should this be backported so that the documentation for older releases is also updated?

This clarifies the `StreamReader.read` behavior in the public
documentation; mostly copying from the more precise docstring of the
function.

While we're at it, also fix some minor grammar in the docstring itself.

Addresses python#100226.
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Feb 10, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@jgosmann
Copy link
Contributor Author

This does not seem to require a NEWS entry to me, but I'm happy to add one if I'm mistaken.

@CAM-Gerlach CAM-Gerlach added docs Documentation in the Doc dir needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Feb 14, 2023
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Standard reminder: You can directly apply all the suggestions you want in one go with Files changed -> Add to batch -> Commit

Sorry for the delay reviewing this. This is certainly an improvement—I'd suggest a number of further clarifications and revisions to fix things that still initially confused me.

Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved
Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved
Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved
Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved
Lib/asyncio/streams.py Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Feb 14, 2023

Should this be backported so that the documentation for older releases is also updated?

Yes; standard practice is to backport any docs-only fixes that affect bugfix-supported branches, as it fixes a docs defect and ensures that other changes can be backported cleanly too. I've added the appropriate tags to line up the backports when this is merged.

This does not seem to require a NEWS entry to me, but I'm happy to add one if I'm mistaken.

No need, you're quite right—it's a docs-only fix.

@gvanrossum
Copy link
Member

No time to review today, but once CAM approves I'll skim and merge.

jgosmann and others added 2 commits February 16, 2023 21:27
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

The main text looks good to me except for on nit.

Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Thanks for checking and correcting the subject matter accuracy of my previous suggestions—I was quite confused by the current text, and my limited background in this area led my guesses for how to clarify the intended meaning to be quite off the mark.

I've suggested a couple followup changes, including seems like it one fairly important apparent omission.

Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved
Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved
Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved

If *n* is not provided, or set to ``-1``,
read until EOF, then return all read :class:`bytes`.
Copy link
Member

Choose a reason for hiding this comment

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

As a beginning reader, before having read @jgosmann 's detailed explaination and @gvanrossum 's summary, I was evidently confused here about what "until EOF" specifically meant in this context. It therefore might warrant some more clarity here, but maybe its just my lack of subject matter expertise compared with the target audience.

jgosmann and others added 2 commits February 17, 2023 18:43
It discusses how many bytes are returned in slightly more detail.
@jgosmann
Copy link
Contributor Author

The macOS build seems to fail for unrelated reasons.

@gvanrossum gvanrossum merged commit 77d95c8 into python:main Feb 17, 2023
@miss-islington
Copy link
Contributor

Thanks @jgosmann for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 17, 2023
@bedevere-bot
Copy link

GH-102001 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Feb 17, 2023
@bedevere-bot
Copy link

GH-102002 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Feb 17, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 17, 2023
@jgosmann jgosmann deleted the streamreader-read-clarifications branch February 17, 2023 21:10
miss-islington added a commit that referenced this pull request Feb 17, 2023
(cherry picked from commit 77d95c8)

Co-authored-by: Jan Gosmann <[email protected]>
@CAM-Gerlach
Copy link
Member

The macOS build seems to fail for unrelated reasons.

Yeah, that's a known unrelated failure, FWIW.

kumaraditya303 pushed a commit that referenced this pull request Feb 23, 2023
…2001)

gh-100226: Clarify StreamReader.read behavior (GH-101807)
(cherry picked from commit 77d95c8)

Co-authored-by: Jan Gosmann <[email protected]>
Co-authored-by: Shantanu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants