Skip to content

/ cpython

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

bpo-39048: Look up __aenter__ before __aexit__ in the async with statement #17609

Merged
merged 5 commits into from Jan 14, 2020
Merged

bpo-39048: Look up __aenter__ before __aexit__ in the async with statement #17609

merged 5 commits into from Jan 14, 2020
+27 −19

Conversation

@maggyero
Copy link
Contributor

maggyero commented Dec 14, 2019 •
edited

This PR will make the following changes:

  • look up the __aenter__ method before the __aexit__ method in the async with statement code;
  • look up the __aenter__ method before the __aexit__ method in the async with statement documentation;
  • add assertions testing that these lookups are performed before executing the body of the async with statement.

https://bugs.python.org/issue39048
@maggyero maggyero changed the title bop-27100: Reorder the __aenter__ and __aexit__ checks for async with bpo-27100: Reorder the __aenter__ and __aexit__ checks for async with Dec 14, 2019
@maggyero maggyero changed the title bpo-27100: Reorder the __aenter__ and __aexit__ checks for async with bpo-39048: Reorder the __aenter__ and __aexit__ checks for async with Dec 14, 2019
@maggyero maggyero changed the title bpo-39048: Reorder the __aenter__ and __aexit__ checks for async with bpo-39048: Reorder the __aenter__ and __aexit__ method checks for the async with statement Dec 14, 2019
@ncoghlan
ncoghlan requested changes Dec 31, 2019
View changes
Copy link
Contributor

ncoghlan left a comment

The change itself looks good to me - just a request for another test case to cover the second error handling branch (if we had already had such a test, we would have picked up the refleak in the previous iteration of the code).

We'll also need a NEWS entry.
Python/ceval.c Show resolved Hide resolved
Lib/test/test_coroutines.py Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 31, 2019

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.
@csabella

This comment has been minimized.

Sign in to view
Copy link
Contributor

csabella commented Jan 12, 2020

@maggyero, please address the code review when you get a chance. Thank you!
@maggyero
Reorder the __aenter__ and __aexit__ checks for async with
f0dbc4b
@maggyero

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

maggyero commented Jan 13, 2020

Sorry @ncoghlan @csabella for the delay, I was on holiday. I am going to address the review today.
@csabella

This comment has been minimized.

Sign in to view
Copy link
Contributor

csabella commented Jan 13, 2020

@maggyero, thank you. I just wanted to make sure you had seen the review.
maggyero added 2 commits Jan 13, 2020
@maggyero
Add assertions for async with body
940e932
@maggyero
Swap __aexit__ and __aenter__ loading in the documentation
Loading status checks…
fe16404
@maggyero maggyero force-pushed the maggyero:patch-14 branch from 0a56e0a to fe16404 Jan 13, 2020
@maggyero maggyero changed the title bpo-39048: Reorder the __aenter__ and __aexit__ method checks for the async with statement bpo-39048: Change the lookup order of __aenter__ and __aexit__ in the async with statement Jan 13, 2020
@maggyero maggyero changed the title bpo-39048: Change the lookup order of __aenter__ and __aexit__ in the async with statement bpo-39048: Change the lookup order of __aenter__ and __aexit__ for the async with statement Jan 13, 2020
@maggyero
Add ACKS and NEWS entries
Loading status checks…
b2d3d83
@maggyero maggyero changed the title bpo-39048: Change the lookup order of __aenter__ and __aexit__ for the async with statement bpo-39048: Change the lookup order of __aenter__ and __aexit__ for async with Jan 13, 2020
@maggyero

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

maggyero commented Jan 13, 2020

I have made the requested changes; please review again.
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 13, 2020

Thanks for making the requested changes!

@ncoghlan: please review the changes made to this pull request.
@bedevere-bot bedevere-bot requested a review from ncoghlan Jan 13, 2020
@rhettinger
rhettinger approved these changes Jan 14, 2020
View changes
@ncoghlan
ncoghlan reviewed Jan 14, 2020
View changes
Misc/NEWS.d/next/Core and Builtins/2020-01-13-14-45-22.bpo-39048.iPsj81.rst Outdated Show resolved Hide resolved
@maggyero
Improve the NEWS entry
Loading status checks…
df005db
@maggyero maggyero requested a review from ncoghlan Jan 14, 2020
@ncoghlan
ncoghlan approved these changes Jan 14, 2020
View changes
@ncoghlan ncoghlan merged commit 1d1b97a into python:master Jan 14, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200114.5 succeeded
Details
bedevere/issue-number Issue number 39048 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ncoghlan

This comment has been minimized.

Sign in to view
Copy link
Contributor

ncoghlan commented Jan 14, 2020

Thank you for the patch!
@maggyero maggyero deleted the maggyero:patch-14 branch Jan 14, 2020
@maggyero maggyero changed the title bpo-39048: Change the lookup order of __aenter__ and __aexit__ for async with bpo-39048: Look up __aenter__ before __aexit__ in async with Jan 14, 2020
@maggyero maggyero changed the title bpo-39048: Look up __aenter__ before __aexit__ in async with bpo-39048: Look up __aenter__ before __aexit__ in the async with statement Jan 14, 2020
@maggyero

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

maggyero commented Jan 14, 2020 •
edited

Thank you for the review @ncoghlan (and @rhettinger)!
@maggyero

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

maggyero commented Jan 14, 2020

@ncoghlan Like for PR #17608, shouldn’t we back port this PR to the 3.8 branch?
@ncoghlan

This comment has been minimized.

Sign in to view
Copy link
Contributor

ncoghlan commented Jan 15, 2020

No, this one isn't eligible for backport as it's a behavioural change, whereas the previous PR was just a docs update.
@maggyero

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

maggyero commented Jan 15, 2020

Alright @ncoghlan!
petdance added a commit to petdance/cpython that referenced this pull request Jan 17, 2020
@maggyero @petdance
bpo-39048: Look up __aenter__ before __aexit__ in async with (pythonG…
25892bc 
…H-17609)

* Reorder the __aenter__ and __aexit__ checks for async with
* Add assertions for async with body being skipped
* Swap __aexit__ and __aenter__ loading in the documentation
petdance added a commit to petdance/cpython that referenced this pull request Jan 17, 2020
@maggyero @petdance
bpo-39048: Look up __aenter__ before __aexit__ in async with (pythonG…
8d70cc8 
…H-17609)

* Reorder the __aenter__ and __aexit__ checks for async with
* Add assertions for async with body being skipped
* Swap __aexit__ and __aenter__ loading in the documentation
petdance added a commit to petdance/cpython that referenced this pull request Jan 17, 2020
@maggyero @petdance
bpo-39048: Look up __aenter__ before __aexit__ in async with (pythonG…
e080939 
…H-17609)

* Reorder the __aenter__ and __aexit__ checks for async with
* Add assertions for async with body being skipped
* Swap __aexit__ and __aenter__ loading in the documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncoghlan ncoghlan

@rhettinger rhettinger

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
6 participants
@maggyero @bedevere-bot @csabella @ncoghlan @rhettinger @the-knights-who-say-ni
You can’t perform that action at this time.