gh-58749: Add test_nonlocal.py to ensure correct behavior of nonlocal statements#126738
gh-58749: Add test_nonlocal.py to ensure correct behavior of nonlocal statements#126738bombs-kim wants to merge 6 commits into
Conversation
|
@ncoghlan Could you give this PR a review? |
There was a problem hiding this comment.
Tests don't need their own news entry.
There was a problem hiding this comment.
Thank you for the feedback. I've removed the Tests NEWS entry file.
|
D'oh, this fell off my radar after #126523 was merged, so the NEWS entry will need some tweaks now :( cc @hugovk since the docs NEWS entry was actually supposed to be there for 3.14.0, so I'm wondering if we should be editing it into the change log for that release rather than whichever maintenance release it ends up landing in (I'll make a wording suggestion based on in going into the 3.15.0 and 3.14.x notes) |
|
I thiunk we can just remove the NEWS entry. It's only increasing the test coverage. If you're worried about where to put the NEWS entry, I'd suggest that we have a separate PR that adds the NEWS entry and then is backported. |
| Corrected the language specification to match CPython's behavior: names in a | ||
| `global` statement are only restricted from being used or assigned prior to | ||
| their declaration in scope. Additional limitations previously suggested by | ||
| the spec do not apply in CPython. | ||
| Change by Beomsoo Kim |
There was a problem hiding this comment.
| Corrected the language specification to match CPython's behavior: names in a | |
| `global` statement are only restricted from being used or assigned prior to | |
| their declaration in scope. Additional limitations previously suggested by | |
| the spec do not apply in CPython. | |
| Change by Beomsoo Kim | |
| 3.14.0 corrected the language specification for the `global` statement to match | |
| CPython's behavior: names in a `global` statement are restricted from being used | |
| or assigned prior to the declaration statement, but can otherwise be bound using | |
| any valid name binding syntax. The previous text erroneously stated that some | |
| name binding syntax (such as loop iteration variables) could not assign to globals. | |
| Change by Beomsoo Kim |
|
We did conclude in the original issue that the docs edit was significant enough that it should have had a NEWS entry. It was sufficiently borderline that I'm not surprised nobody complained about it (it was one of those cases where what everyone assumed the spec said was not, in fact, what the text actually contained), so I won't argue for it if we decide that the inadvertent omissions makes it no longer worth worrying about. |
|
Maybe I phrased it poorly, but this specific PR does not need a NEWS entry. I think it's easier if we have another PR that would just add the NEWS entry wherever needed, whether it's directly in the 3.14.0 section or in the 3.14.2 section so that it's at the "top" of the page (though, when it's 3.14.3, it won't be at the top anymore so... it doesn't really matter that we put it manually in 3.14.0) |
|
This PR is stale because it has been open for 30 days with no activity. |
As suggested by @ncoghlan:
test_nonlocal.pyIn tests, I've decided to use simply the variable names, rather than using
f.__closure__[0].cell_contents. It seems to me that both approaches can ensure the correct behavior of nonlocal statements and simply using the variable names would make the test code easier to understand. I will change it back tof.__closure__[0].cell_contentsif this is not desirable.