-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
bpo-29622: make AST constructor accepts less than enough number of positional arguments #249
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
Conversation
Parser/asdl_c.py
Outdated
@@ -666,11 +666,10 @@ def visitModule(self, mod): | |||
} | |||
res = 0; /* if no error occurs, this stays 0 to the end */ | |||
if (PyTuple_GET_SIZE(args) > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
I implemented minimum number of arguments. |
Could I ask that we don't make arguments required, even if you need them to construct a valid AST? I have a pair of little side projects (astsearch and astcheck) which rely on the ability to instantiate incomplete AST objects. I reuse these as templates, so you can check for things like 'for loop with an else clause' without specifying any of the required pieces of a for loop. Obviously this isn't what the AST classes are intended for, but it works pretty nicely. And yes, I am being this guy: |
@takluyver Thank you. I decided to revert last commit. |
Thanks @methane :-) |
@@ -373,12 +373,8 @@ def test_nodeclasses(self): | |||
self.assertEqual(x.right, 3) | |||
self.assertEqual(x.lineno, 0) | |||
|
|||
# node raises exception when not given enough arguments | |||
self.assertRaises(TypeError, ast.BinOp, 1, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep these and change to assertNotRaises ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, too late, that was merged while I was reviewing :-)
for (i = 0; i < PyTuple_GET_SIZE(args); i++) { | ||
/* cannot be reached when fields is NULL */ | ||
PyObject *name = PySequence_GetItem(fields, i); | ||
if (!name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity what's the prefered python style !variable
or == NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't care about !name
or name == NULL
. And I only removed indent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I saw. It was a genuine question like if you are coding something new, what's the prefered style in the CPython codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.python.org/dev/peps/pep-0007/
PEP 7 doesn't define it. But I think consistency in one file is recommended like PEP 7.
Thanks ! |
…n API Stackless does not support custom frame evaluation functions defined by PEP 523. If an extension module sets a custom frame evaluation function, Stackless now terminates to prevent undefined behavior. (cherry picked from commit d57b317)
…n API Stackless does not support custom frame evaluation functions defined by PEP 523. If an extension module sets a custom frame evaluation function, Stackless now terminates to prevent undefined behavior.
…athsep Honor '/'-separated names in ResourceContainer.joinpath.
Currently, AST constructor accepts
a. empty arguments
b. positional arguments, only when it's length is exactly same to number of fields
c. keyword arguments. No check for missing required fields.
Only (b) is strict. And it require argument even if matching field is optional.
This pull request removes the strict check.
Missing required field can be detected when compiling AST though.