-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Improve stub files generated by stubgenc for pybind11 modules #5850
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
Improve stub files generated by stubgenc for pybind11 modules #5850
Conversation
Strip module name from type information if its the same as annotated module or add import statement.
Please fix the tests (see https://travis-ci.org/python/mypy/jobs/447561624).
Also add some tests for the new functionality. We really need to have more
tests for stubgenc.
|
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.
Sorry for the late review -- this got caught in a busy period. I have a few requests for changes, but in general I think this is fine. (I do wonder if maybe we should switch to a more robust approach to handling argument lists rather than repeated splitting and stripping. But that's for a follow-up PR.)
mypy/stubgenc.py
Outdated
|
||
if sig: | ||
sig_types = [] | ||
for arg in (arg.split(':', 1) for arg in sig.split(',')): |
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'm sorry, but this code is inscrutable, and I have no easy example in the stdlib to even test it (no C function in the stdlib has type annotations in its docstring). At the very least please don't use the same name (arg
) for the two different variables -- the inner arg
takes on string values like x: int
or foo: C
while the outer arg
would take on list values like ['x', ' int']
and ['foo', ' C']
. You might also want to add some comments explaining the different cases (len(arg)
being 1 or not).
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.
(Sorry, marking as "changes requested".)
I hope that I correctly identified changes that you've requested. The only thing that may require further discussion is handling of |
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.
Everything looks good except I'd still like to see some improvements on the "inscrutable" for-loop. :-)
Thinking it through I believe the special case for pybind11_object is acceptable -- I assume you don't have a say in what pybind11 generates anyway...
mypy/stubgenc.py
Outdated
if len(arg) == 1: | ||
sig_types.append(arg[0].strip()) | ||
# convert signature in form of "self: TestClass, arg0: str" to list [[self, TestClass], [arg0, str]] | ||
for arg_type in (arg.split(':', 1) for arg in sig.split(',')): |
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 rewrite this as follows:
# Convert signature in form of "self: testmod.TestClass, arg0: str" to list ["self: testmod.TestClass", "arg0: str"]
# and update imports with relevant modules (e.g. "testmod").
for arg in sig.split(','):
arg_type = arg.split(':', 1)
if len(arg_type) == 1:
# etc.
* Fix PEP8 line length * simplify logic * introduce testcase for generate_c_function_stub that verifies docstring parsing
LGTM, waiting for tests (I fixed one typo). |
Thank you! |
Following changes fixes stubgenc that generates invalid *pyi files in following cases:
I'm not sure about 2, maybe pybind11 library should provide dummy pybind11_object in its module and annotate types, so dummy object will be assumed as superclass.