-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-64490: Argument Clinic: Add support for **kwds
#138344
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
This adds a scaffold of support, initially only working with strictly positional-only arguments. The FASTCALL calling convention is not yet supported.
Do you plan to convert some functions to Argument Clinic thanks for this change? Or is it just for completeness? |
Yes, but I didn't want to make this PR bigger by including them. A |
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
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.
Should we test when someone uses **kwds: object
or **kwds: int
? Maybe we should enforce that **kwds
is always annotated with "dict"? Also, I think more tests should be added for bad usage (for *args
, the coverage is quite large, see test_clinic.py).
In addition, I would suggest using __clinic_kwds
instead of kwds
for the local variable when we have the (... *args, **kwargs)
as someone could define kwds
as being one of the positional arguments. This is what we do with args
where we define __clinic_args
if there are non-variadic parameters.
Those are just suggestions for fortifying the current interface though so they can be addressed later.
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.
In general, looks right.
We need tests in Python that use functions defined in Modules/_testclinic.c
. We need tests for different invalid combinations (var-keyword after keyword-or-positional or keyword-only parameters, different parameters after the var-keyword parameter, wrong converter for the var-keyword parameter, default value for the var-keyword parameter, the var-keyword parameter with groups, the var-keyword parameter with the defining_class parameter).
# Conflicts: # Lib/test/test_clinic.py
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.
Thanks, looks good!
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.
Thank you for addressing all my comments. It seems that the tests have not expose any bugs in code. Few more suggestions and nitpicks.
[ | ||
**kwds: dict | ||
] |
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.
While we are here, could you also add a case with a var-positional parameter inside an optional group?
And I think that it would be worth to add a test for different kinds of parameters (keyword-or-positional, keyword-only, var-positional, var-keyword) after a valid optional group (if there are no such tests yet). This may be a separate test method. Some of these cases can be supported in principle in future. We want to know if we enabled them accidentally.
This adds a scaffold of support, initially only working with strictly positional-only arguments. The FASTCALL calling convention is not yet supported.
I expect future PRs to relax these constraints.
Currently supported forms:
f(**kwds)
f(a, b, /, **kwds)
f(*args, **kwds)
f(a, b, /, *args, **kwds)
A