Skip to content

Conversation

@anordin95
Copy link
Contributor

@anordin95 anordin95 commented Aug 22, 2025

@bedevere-app
Copy link

bedevere-app bot commented Aug 26, 2025

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.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

With Hugo's suggested title changes, I am fine with the current state of this document.

Let's strive to keep this conceptual overview fairly high-level. The doc's purpose should stay focused on an overview and avoid going too far into a technical deep dive.

@anordin95
Copy link
Contributor Author

anordin95 commented Sep 3, 2025

I think all requested changes have been addressed in this PR, besides modifications to the await task and await coroutine section titles requested by @hugovk & @willingc.

For ease of reference, the style guide says: "In the Python documentation, the use of sentence case in section titles is preferable, but consistency within a unit is more important than following this rule.".
I think it's fair to say these titles are consistent with the broader unit title: await.

Additionally, I think the use of "preferable" above implies there can be reasonable cases that ignore this guidance. I personally prefer the titles as they are now, for the reasons enumerated here.

With that in mind, I wonder if leaving them as is would be amenable for folks?

@hugovk
Copy link
Member

hugovk commented Sep 4, 2025

Please do rename them to "Awaiting tasks" and "Awaiting coroutines".

For ease of reference, the style guide says: "In the Python documentation, the use of sentence case in section titles is preferable, but consistency within a unit is more important than following this rule.".
I think it's fair to say these titles are consistent with the broader unit title: await.

That's referring to the await keyword, so is lowercase. Should it be in code formatting, and/or reworded?

The next sentence of the devguide:

If you add a section to a chapter where most sections are in title case, you can either convert all titles to sentence case or use the dominant style in the new section title.

image

Well, the rest is already in sentence case, let's stick to it.

@anordin95
Copy link
Contributor Author

anordin95 commented Sep 4, 2025

For reference, I interpreted the style guide as providing some leeway and more generally I think it can make sense to sometimes ignore strict adherence to style guides, in this case, for the sake of content/flow.

Either way, it seems my arguments fell short and failed to sway minds! I'll go ahead and change the section titles as requested.

P.S. The same idea regarding await came up in the original PR. Making it a code-block means it will be bolded in the table of contents on the sidebar, so we decided to leave it as is.

@anordin95
Copy link
Contributor Author

Hey folks, I believe all comments have now been addressed.

@willingc, @kumaraditya303, @ZeroIntensity would you mind taking another pass?

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Mostly LGTM

Then, the event loop needs to manage its internal state and work through
its processing logic to resume the next job.
That might sound minor, but in a large program with many ``await``\ s, that
overhead can add up to a meaningful performance drag.
Copy link
Member

Choose a reason for hiding this comment

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

I think "meaningful" has a positive connotation here, which we don't want. How about this?

Suggested change
overhead can add up to a meaningful performance drag.
overhead can add up to a major performance drag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make sure I understand, the concern is that "meaningful" has a positive connotation, which may confuse a reader into thinking we're saying a "performance drag" is a good thing, yeah?

I think "major" would be overstating the effect. The goal was to describe something that's non-trivial or non-negligible. For what it's worth, I think "meaningful" can be used either way e.g. "a meaningful decline in sales", "a meaningful improvement in test scores", etc. Regardless, would switching to "non-trivial" be amenable?

Copy link
Member

Choose a reason for hiding this comment

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

To me, "non-trivial" would imply "complex", which doesn't really fit here. I'd prefer "non-negligible".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@anordin95
Copy link
Contributor Author

@kumaraditya303, I believe this PR was modified to address your requested changes. Could you update that "requested changes" status so we can unblock this PR?

@anordin95
Copy link
Contributor Author

anordin95 commented Nov 17, 2025

@ZeroIntensity, @willingc - Would y'all mind giving this another pass or approving if it looks good? :)

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks @anordin95. I'm going to go ahead and merge. 🎉

@willingc willingc merged commit b3b63e8 into python:main Nov 19, 2025
32 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Docs PRs Nov 19, 2025
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

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants