-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Allow star args in ctypes.Array constructor #6213
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
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! PR looks good, I just have few suggestions for additional tests.
mypy/plugins/ctypes.py
Outdated
for arg_num, (arg_kind, arg_type) in enumerate(zip(ctx.arg_kinds[0], ctx.arg_types[0]), 1): | ||
if arg_kind not in {nodes.ARG_POS, nodes.ARG_STAR}: | ||
ctx.api.msg.fail( | ||
"Array constructor does not allow keyword arguments", |
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.
Please add a test that triggers this error message.
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 like this actually isn't ever called -- it looks like the constructor in typeshed def __init__(self, *args: Any) -> None)
definition takes care of this case.
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.
Oh, wait, so if it is never going to be called, maybe we can remove this code?
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.
Done!
if not is_subtype(arg_type, ty): | ||
ctx.api.msg.fail( | ||
'Array constructor argument {} of type "{}"' | ||
' is not convertible to the array element type "Iterable[{}]"' |
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.
Please also add a test for this error message.
[case testCtypesArrayConstructorKwargs] | ||
import ctypes | ||
intarr4 = ctypes.c_int * 4 | ||
|
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 wanted to add a intarr4(1, 2, 3, x=4)
test case here, but not sure how to get the tests to pass: the output on my machine is:
Expected:
main:4: error: Unexpected keyword argument "x" for "Array"
main:7: error: Too many arguments for "Array" (diff)
Actual:
main:4: error: Unexpected keyword argument "x" for "Array"
/home/alan/workspace/python/mypy/mypy/typeshed/stdlib/2and3/ctypes/__init__.pyi:273: note: "Array" defined here (diff)
main:7: error: Too many arguments for "Array" (diff)
I added a # E:
comment for the error, but don't know how to handle the note (which includes the path, so will be setup specific). Thoughts?
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.
Oh, this note caused some problems before. One option would be to add some very simple mock stubs to test-data/unit/lib-stub
. But probably even better is just not add this test. The two tests you have added are sufficient.
### Description Closes #5580 Previously, the type of empty dicts are inferred as `dict[<nothing>, <nothing>]`, which is not a subtype of `Mapping[str, Any]`, and this has caused a false-positive error saying `Keywords must be strings`. This PR fixes it by inferring the types of double-starred arguments with a context of `Mapping[str, Any]`. Closes #4001 and closes #9007 (duplicate) Do not check for "too many arguments" error when there are any double-starred arguments. This will lead to some false-negavites, see my comment here: #4001 (comment) ### Test Plan Added a simple test `testPassingEmptyDictWithStars`. This single test can cover both of the issues above. I also modified some existing tests that were added in #9573 and #6213.
No description provided.