X Tutup
Skip to content

bpo-38469: Handle named expression scope whith global/nonlocal keywords#16755

Merged
pablogsal merged 4 commits intopython:masterfrom
pablogsal:bpo-38469
Oct 14, 2019
Merged

bpo-38469: Handle named expression scope whith global/nonlocal keywords#16755
pablogsal merged 4 commits intopython:masterfrom
pablogsal:bpo-38469

Conversation

@pablogsal
Copy link
Copy Markdown
Member

@pablogsal pablogsal commented Oct 13, 2019 •
edited by bedevere-bot
Loading

@pablogsal pablogsal force-pushed the bpo-38469 branch 4 times, most recently from 5db3b99 to 319b38d Compare October 13, 2019 22:51
@pablogsal pablogsal self-assigned this Oct 13, 2019
gvanrossum
gvanrossum reviewed Oct 13, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Did you force-push anything? GitHub told me to refresh the PR but there was no new commit.

Also, alas, I don't feel comfortable approving this, I don't know the symtable code well enough any more. Sorry!

@pablogsal
Copy link
Copy Markdown
Member Author

pablogsal commented Oct 14, 2019

Did you force-push anything? GitHub told me to refresh the PR but there was no new commit.

Yeah, I force pushed a version just some minutes ago. Sorry for the confusion.

@pablogsal
Copy link
Copy Markdown
Member Author

pablogsal commented Oct 14, 2019

Also, alas, I don't feel comfortable approving this, I don't know the symtable code well enough any more. Sorry!

Not a problem, thanks for the review! :)

@ncoghlan
Copy link
Copy Markdown
Contributor

ncoghlan commented Oct 14, 2019

There should be existing test cases for this, so the question is "What's wrong with them"?

The testing style in the module is also to rely on exec(), rather than on the test module itself (otherwise a compile error will keep the entire test file from running)

@ncoghlan
Copy link
Copy Markdown
Contributor

ncoghlan commented Oct 14, 2019

Sorry, I retract that "exec" comment - it only applies to the tests that are explicitly checking for compilation errors

@ncoghlan
Copy link
Copy Markdown
Contributor

ncoghlan commented Oct 14, 2019

And the missing test case is also genuinely missing rather than existing in a flawed form (the global declaration test doesn't use a comprehension, the comprehension tests don't use a global)

@pablogsal @ncoghlan
Update Misc/NEWS.d/next/Core and Builtins/2019-10-13-23-41-38.bpo-384…
df21d12 
…69.9kmuQj.rst

Co-Authored-By: Nick Coghlan <ncoghlan@gmail.com>
@pablogsal
Copy link
Copy Markdown
Member Author

pablogsal commented Oct 14, 2019

Thanks @ncoghlan for the thorough review!

@pablogsal pablogsal merged commit fd5c414 into python:master Oct 14, 2019
@pablogsal pablogsal deleted the bpo-38469 branch October 14, 2019 04:18
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Oct 14, 2019

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 14, 2019
@pablogsal @miss-islington
bpo-38469: Handle named expression scope with global/nonlocal keywords (
63983da 
pythonGH-16755)

The symbol table handing of PEP572's assignment expressions is not resolving correctly the scope of some variables in presence of global/nonlocal keywords in conjunction with comprehensions.
(cherry picked from commit fd5c414)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Oct 14, 2019

GH-16760 is a backport of this pull request to the 3.8 branch.

ambv pushed a commit that referenced this pull request Oct 14, 2019
@miss-islington @pablogsal @ambv
bpo-38469: Handle named expression scope with global/nonlocal keywords (
927f07c 
GH-16755) (#16760)

The symbol table handing of PEP572's assignment expressions is not resolving correctly the scope of some variables in presence of global/nonlocal keywords in conjunction with comprehensions.
(cherry picked from commit fd5c414)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@pablogsal
bpo-38469: Handle named expression scope with global/nonlocal keywords (
9562587 
pythonGH-16755)

The symbol table handing of PEP572's assignment expressions is not resolving correctly the scope of some variables in presence of global/nonlocal keywords in conjunction with comprehensions.
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@pablogsal @shihai1991
bpo-38469: Handle named expression scope with global/nonlocal keywords (
fe8813d 
pythonGH-16755)

The symbol table handing of PEP572's assignment expressions is not resolving correctly the scope of some variables in presence of global/nonlocal keywords in conjunction with comprehensions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments

@ncoghlan ncoghlan ncoghlan approved these changes

@emilyemorehouse emilyemorehouse Awaiting requested review from emilyemorehouse

Assignees

@pablogsal pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pablogsal @ncoghlan @miss-islington @bedevere-bot @gvanrossum @the-knights-who-say-ni
X Tutup