Skip to content

gh-125289: Update sample code in asyncio-task.rst #125292

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.

Sign up for GitHub

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

Jump to bottom
Merged
willingc merged 3 commits into from
Oct 12, 2024
Merged

gh-125289: Update sample code in asyncio-task.rst #125292

willingc merged 3 commits into from
Oct 12, 2024

Conversation

@galmyk
Copy link
Contributor

@galmyk galmyk commented Oct 11, 2024 •
edited by github-actions bot
Loading

gh-125289: Update sample code in asyncio-task.rst

This will change coroutines sample code in the Awaitables section and make the example clearer.

📚 Documentation preview 📚: https://cpython-previews--125292.org.readthedocs.build/

@galmyk
Update sample code in asyncio-task.rst
da52b65 
This will change **coroutines** sample code in the **Awaitables** section and make the example clearer.
@ghost
Copy link

ghost commented Oct 11, 2024 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app bedevere-app bot added docs Documentation in the Doc dir skip news labels Oct 11, 2024
@bedevere-app bedevere-app bot mentioned this pull request Oct 11, 2024
Closed
JacobCoffee
JacobCoffee approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@JacobCoffee JacobCoffee left a comment

Choose a reason for hiding this comment

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

this lgtm, but more broadly: i find this example weird in general that if i as a new user copy and paste it it just throws an error.

i think it would be nice to scour the documentation and "split" examples into working/non-working and denote them as such... @willingc thoughts?

@galmyk
Copy link
Contributor Author

galmyk commented Oct 11, 2024

@JacobCoffee I agree with splitting working and not working codes. But in this case, code works, just prints a warning:

/home/gmt/a.py:10: RuntimeWarning: coroutine 'nested' was never awaited
  nested()  # will raise a "RuntimeWarning".
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
42

I'm not native english speaker, So I fear my wording in the following line's comment is not correct:

ested()  # will raise a "RuntimeWarning".

It just prints the warning! If it is wrong, please propose a better comment.
Or is it better to remove that command?

@willingc
Copy link
Contributor

willingc commented Oct 11, 2024

this lgtm, but more broadly: i find this example weird in general that if i as a new user copy and paste it it just throws an error.

i think it would be nice to scour the documentation and "split" examples into working/non-working and denote them as such... @willingc thoughts?

Hmm... there are a couple of concepts at work here: async/await, nesting coroutines, and example behavior

I think the use of main() provides a bit more confusion since it is an async function in the example to illustrate nesting. It may be better to use something other than main maybe outer().

Where print() is placed is a bit of a red herring, since it's the async/await nesting that governs behavior.

I do think it makes sense to improve this section of docs, but I want to make sure that we do it in a way that gets the point across clearly about nesting.

@galmyk
Copy link
Contributor Author

galmyk commented Oct 12, 2024

@willingc , @JacobCoffee
Isn't better to merge this with minimal corrections, and add a more complete change to backlog? I think we need to check other code examples, too.

willingc
willingc reviewed Oct 12, 2024
View reviewed changes
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.

Let's revert the change in print and limit this PR to adding the runtime error comment.

galmyk and others added 2 commits October 12, 2024 22:23
@galmyk @willingc
Update Doc/library/asyncio-task.rst
5bbb942 
Revert the added print

Co-authored-by: Carol Willing <[email protected]>
@galmyk @willingc
Update Doc/library/asyncio-task.rst
7e3cd87 
Co-authored-by: Carol Willing <[email protected]>
willingc
willingc approved these changes Oct 12, 2024
View reviewed changes
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 @galmyk for the PR and @JacobCoffee for the initial review and discussion.

@willingc willingc added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Oct 12, 2024
@willingc willingc merged commit fa52b82 into python:main Oct 12, 2024
@miss-islington-app
Copy link

miss-islington-app bot commented Oct 12, 2024

Thanks @galmyk for the PR, and @willingc for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 12, 2024
@galmyk @willingc @miss-islington
pythongh-125289: Update sample code in asyncio-task.rst (pythonGH-125292
8022806 
)

* Update sample code in asyncio-task.rst

This will change **coroutines** sample code in the **Awaitables** section and make the example clearer.

* Update Doc/library/asyncio-task.rst

Revert the added print

Co-authored-by: Carol Willing <[email protected]>

* Update Doc/library/asyncio-task.rst

Co-authored-by: Carol Willing <[email protected]>

---------

(cherry picked from commit fa52b82)

Co-authored-by: Ghorban M. Tavakoly <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 12, 2024
@galmyk @willingc @miss-islington
pythongh-125289: Update sample code in asyncio-task.rst (pythonGH-125292
92effbb 
)

* Update sample code in asyncio-task.rst

This will change **coroutines** sample code in the **Awaitables** section and make the example clearer.

* Update Doc/library/asyncio-task.rst

Revert the added print

Co-authored-by: Carol Willing <[email protected]>

* Update Doc/library/asyncio-task.rst

Co-authored-by: Carol Willing <[email protected]>

---------

(cherry picked from commit fa52b82)

Co-authored-by: Ghorban M. Tavakoly <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 12, 2024

GH-125374 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 12, 2024
@bedevere-app
Copy link

bedevere-app bot commented Oct 12, 2024

GH-125375 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 12, 2024
willingc added a commit that referenced this pull request Oct 12, 2024
@miss-islington @galmyk @willingc
[3.13] gh-125289: Update sample code in asyncio-task.rst (GH-125292) (G…
6afdb09 
…H-125374)

gh-125289: Update sample code in asyncio-task.rst (GH-125292)

* Update sample code in asyncio-task.rst

This will change **coroutines** sample code in the **Awaitables** section and make the example clearer.

* Update Doc/library/asyncio-task.rst

Revert the added print



* Update Doc/library/asyncio-task.rst



---------

(cherry picked from commit fa52b82)

Co-authored-by: Ghorban M. Tavakoly <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
willingc added a commit that referenced this pull request Oct 12, 2024
@miss-islington @galmyk @willingc
[3.12] gh-125289: Update sample code in asyncio-task.rst (GH-125292) (G…
1d45fae 
…H-125375)

gh-125289: Update sample code in asyncio-task.rst (GH-125292)

* Update sample code in asyncio-task.rst

This will change **coroutines** sample code in the **Awaitables** section and make the example clearer.

* Update Doc/library/asyncio-task.rst

Revert the added print



* Update Doc/library/asyncio-task.rst



---------

(cherry picked from commit fa52b82)

Co-authored-by: Ghorban M. Tavakoly <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@willingc willingc willingc approved these changes

@JacobCoffee JacobCoffee JacobCoffee approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@galmyk @willingc @JacobCoffee