Skip to content

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

Merged
merged 3 commits into from
Feb 23, 2017

Conversation

methane
Copy link
Member

@methane methane commented Feb 23, 2017

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.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is redundant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@methane methane changed the title bpo-29622: Loosen restriction of number of positional argument of AST constructor bpo-29622: Make trailing optional fields to optional arguments of AST type Feb 23, 2017
@methane
Copy link
Member Author

methane commented Feb 23, 2017

I implemented minimum number of arguments.
But I still don't know should I check it, since AST constructor doesn't check it when keyword argument is used.

@takluyver
Copy link
Contributor

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:

xkcd 'Workflow'

@methane
Copy link
Member Author

methane commented Feb 23, 2017

@takluyver Thank you. I decided to revert last commit.

@takluyver
Copy link
Contributor

Thanks @methane :-)

@methane methane changed the title bpo-29622: Make trailing optional fields to optional arguments of AST type bpo-29622: make AST constructor accept less than enough number of positional arguments Feb 23, 2017
@methane methane changed the title bpo-29622: make AST constructor accept less than enough number of positional arguments bpo-29622: make AST constructor accepts less than enough number of positional arguments Feb 23, 2017
@methane methane merged commit 4c78c52 into python:master Feb 23, 2017
@@ -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)
Copy link
Contributor

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 ?

Copy link
Contributor

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) {
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

@Carreau
Copy link
Contributor

Carreau commented Feb 23, 2017

Thanks !

@methane methane deleted the bpo-29622 branch January 29, 2018 06:05
akruis pushed a commit to akruis/cpython that referenced this pull request May 21, 2021
…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)
akruis pushed a commit to akruis/cpython that referenced this pull request May 27, 2021
…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.
jaraco pushed a commit that referenced this pull request Dec 2, 2022
jaraco added a commit to jaraco/cpython that referenced this pull request Feb 17, 2023
…athsep

Honor '/'-separated names in ResourceContainer.joinpath.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants