-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
bpo-36290: Fix kwargs handling in ast node constructors #12382
bpo-36290: Fix kwargs handling in ast node constructors #12382
Conversation
@matrixise, sorry I missed Zackery comment, I will fix this when I get home. |
@remilapeyre Nice, I wanted to check all of your PRs. |
Hi @matrixise, I've made the requested changes and resolved the conflicts, it should be ready for a new review :) |
Misc/NEWS.d/next/Library/2019-03-17-19-01-53.bpo-36290.7VXo_K.rst
Outdated
Show resolved
Hide resolved
Co-Authored-By: Batuhan Taşkaya <isidentical@gmail.com>
Hi @isidentical, thanks for the review, it should be good now. |
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.
It looks OK (except a few minor nits) but I wonder why you choose EAFP rather then LBYL for this case. I guess you can just do a contains check, no? and it would look simpler then the current version.By the way thanks for your active work on PR.
I'm not sure what you mean in this context, no exception is raised so I don't see how I could ask for forgiveness. I could have used a contains check on line 714, is this what you mean? It does look better I think, I can make the change if so. |
Sorry about my conversation skills, I'm still trying to improve them. I mean that instead of that def _new(cls, *args, **kwargs):
for key in kwargs:
if key not in cls._fields:
continue # arbitrary keyword arguments are accepted
pos = cls._fields.index(key)
if pos < len(args):
raise TypeError(f"{cls.__name__} got multiple values "
f"for argument {key!r}")
if cls in _const_types:
return Constant(*args, **kwargs)
return Constant.__new__(cls, *args, **kwargs) Something like this is what I meant (of course also in the C implementation). What do you think about this? |
Ok, I agree it make the Python code better but for the C version all return codes need to be checked anyway so I'm not sure it's better: i = 0; /* needed by PyDict_Next */
while (PyDict_Next(kw, &i, &key, &value)) {
int contains = PySequence_Contains(fields, key);
if (contains == -1) {
res = -1;
goto cleanup;
} else if (contains == 0) {
continue;
}
Py_ssize_t p = PySequence_Index(fields, key);
if (p == -1) {
res = -1;
goto cleanup;
}
else {
if (p < PyTuple_GET_SIZE(args)) {
PyErr_Format(PyExc_TypeError,
"%.400s got multiple values for argument '%U'",
Py_TYPE(self)->tp_name, key);
res = -1;
goto cleanup;
}
}
res = PyObject_SetAttr(self, key, value);
if (res < 0) {
goto cleanup;
}
} |
I dont think it is good idea to assume ValueError only comes when the item is not present, it might come from something else in the future (unlikely, but why dont we make this more strict if we have a chance). |
That makes sense, here's a new patch without this issue. |
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.
Looks great, it might worth to consider to add a case about arbitrary keyword arguments (both pseudo compatibility nodes and real nodes)
🤖 New build scheduled with the buildbot fleet by @isidentical for commit 1e1d3b5 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
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.
LGTM
Thanks @remilapeyre for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9. |
…ythonGH-12382) (cherry picked from commit c73914a) Co-authored-by: Rémi Lapeyre <remi.lapeyre@lenstra.fr>
GH-20365 is a backport of this pull request to the 3.9 branch. |
Thanks for the PR @remilapeyre and thanks for the review @isidentical ! |
…ythonGH-12382) (cherry picked from commit c73914a) Co-authored-by: Rémi Lapeyre <remi.lapeyre@lenstra.fr>
GH-20366 is a backport of this pull request to the 3.8 branch. |
Thank you for your continous work on this PR @remilapeyre and thanks for the merge @pablogsal 🎊 🥳 |
https://bugs.python.org/issue36290