gh-137600: Promote ast node constructor deprecation warnings to errors#137601
gh-137600: Promote ast node constructor deprecation warnings to errors#137601brianschubert wants to merge 22 commits intopython:mainfrom
ast node constructor deprecation warnings to errors#137601Conversation
brianschubert
requested review from
AA-Turner,
Eclips4,
JelleZijlstra,
isidentical and
tomasr8
as code owners
|
@picnixz As mentioned in #121162 (comment), now that the constructors raise on missing and unknown fields, some of the checks that were added for |
|
This takes me back as the original PR is one of my very first contributions to CPython! |
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
I'd suggest using msg = ... + re.escape() for assertRaisesRegex() where 80 chars limit is exceeded.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
@picnixz do you have any further feedback? |
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
I do not but I actually had a pending review that I started 1 month ago. They are mainly for improving coverage and wording but they are optional.
Misc/NEWS.d/next/Core_and_Builtins/2025-08-09-19-00-36.gh-issue-137600.p_p6OU.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
@JelleZijlstra friendly ping! Anything else needed here? |
JelleZijlstra
left a comment
JelleZijlstra
left a comment
There was a problem hiding this comment.
Thanks, left some suggestions for the tests.
Lib/test/test_ast/test_ast.py
Outdated
| # Custom attribute assignment is allowed | ||
| x.foo = 5 | ||
| self.assertEqual(x.foo, 5) | ||
| del x.foo |
There was a problem hiding this comment.
Why is this needed? Doesn't the object get deleted anyway?
There was a problem hiding this comment.
fair point, I guess at the time I was trying to test that deletion is well behaved too? I don't see much precedence for testing that elsewhere in the tests, so I think I'll remove it
Lib/test/test_ast/test_ast.py
Outdated
|
|
||
| with self.assertRaises(AttributeError): | ||
| x.value | ||
| msg = "ast.Constant.__init__ missing 1 required positional argument: 'value'" |
There was a problem hiding this comment.
This test now feels out of scope for the test function. If we're already sufficiently testing that this case errors out (I assume we are), let's just remove this line.
There was a problem hiding this comment.
Agreed, this doesn't have anything to do the node attributes anymore (I should have read the test name!). I'll remove it.
Lib/test/test_ast/test_ast.py
Outdated
| # Arbitrary keyword arguments are supported (but deprecated) | ||
| with self.assertWarns(DeprecationWarning): | ||
| self.assertEqual(ast.Constant(1, foo='bar').foo, 'bar') | ||
| # Arbitrary keyword arguments are not supported |
There was a problem hiding this comment.
Same here, this is now testing something unrelated to what this function is about.
There was a problem hiding this comment.
My intent here was to be sure we were ignoring kwargs.
There was a problem hiding this comment.
Agreed as well, I'll remove it.
Not accepting arbitrary kwargs has coverage elsewhere, e.g. here:
https://github.com/brianschubert/cpython/blob/f3e21e6a37d5e479d4d6f597544b2036f6a6ae3d/Lib/test/test_ast/test_ast.py#L588
4 participants
astnode constructor deprecation warnings to errors #137600📚 Documentation preview 📚: https://cpython-previews--137601.org.readthedocs.build/