gh-137600: Promote ast node constructor deprecation warnings to errors
#137601
Conversation
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 <[email protected]>
|
@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.
| self.assertEqual(x.right, n3) | ||
|
|
||
| # Random attribute allowed too | ||
| x.foobarbaz = 5 |
There was a problem hiding this comment.
I don't know if we already cover this, but maybe we should instead:
- Find a way to construct all nodes "correctly" or have at least one instance of each nodes.
- For all these instances, check that we can set random attributes and delete them properly.
There was a problem hiding this comment.
Good idea, added a test in the style of the existing test_field_attr_existence test.
| The constructors of node types in the :mod:`ast` module now raise a | ||
| :exc:`TypeError` when a required argument is omitted or when a | ||
| keyword argument that does not map to a field on the AST node is passed. | ||
| These cases had previously raised a :exc:`DeprecationWarning` since Python | ||
| 3.13. Patch by Brian Schubert. |
There was a problem hiding this comment.
| The constructors of node types in the :mod:`ast` module now raise a | |
| :exc:`TypeError` when a required argument is omitted or when a | |
| keyword argument that does not map to a field on the AST node is passed. | |
| These cases had previously raised a :exc:`DeprecationWarning` since Python | |
| 3.13. Patch by Brian Schubert. | |
| :mod:`ast`: The constructors of AST nodes now raise a | |
| :exc:`TypeError` when a required argument is omitted or when a | |
| keyword argument that does not map to a field on the AST node is passed. | |
| These cases had previously raised a :exc:`DeprecationWarning` since Python | |
| 3.13. Patch by Brian Schubert. |
And you can reflow the paragraph as you deem fit (and also you can reuse that message in the NEWS).
There was a problem hiding this comment.
Updated, thanks!
Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
|
@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.
| # 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?
|
|
||
| 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.
| # 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.
4 participants
astnode constructor deprecation warnings to errors #137600📚 Documentation preview 📚: https://cpython-previews--137601.org.readthedocs.build/