Skip to content
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

Fixed stubgen parsing generics from C extensions #8939

Merged
merged 1 commit into from
Aug 14, 2020

Conversation

AWhetter
Copy link
Contributor

@AWhetter AWhetter commented Jun 3, 2020

pybind11 is capable of producing type signatures that use generics (for example https://github.com/pybind/pybind11/blob/4e3d9fea74ed50a042d98f68fa35a3133482289b/include/pybind11/stl.h#L140). A user may also opt to write a signature in the docstring that uses generics.
Currently when stubgen parses one of these generics, it attempts to import a part of it. For example if a docstring had my_func(str, int) -> List[mypackage.module_being_parsed.MyClass], the resulting stub file tries to import List[mypackage.module_being_parsed.
This change fixes this behaviour by breaking the found type down into the multiple types around [], characters, adding any imports from those types that are needed, and then stripping out the name of the module being parsed.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks! Looks reasonable, but I'd love to see some tests for this, as otherwise a future contributor might accidentally break something.

r'(^|[\[, ]+)' + re.escape(module.__name__ + '.'),
r'\1',
typ,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have some unit tests for the new functionality, since this logic is non-trivial. You can add them to mypy/test/teststubgen.py.

(Maybe also modify this so that the module name is passed instead of a module object.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I couldn't find the tests for stubgen. I'll get those added. Thanks for the review!

@AWhetter AWhetter force-pushed the fix_stubgen_generic_import branch from 0305fab to 64ebff3 Compare July 2, 2020 19:31
@AWhetter AWhetter force-pushed the fix_stubgen_generic_import branch from 64ebff3 to cf31345 Compare July 12, 2020 18:31
@AWhetter AWhetter force-pushed the fix_stubgen_generic_import branch from cf31345 to 0c3a7bd Compare July 31, 2020 15:37
@AWhetter
Copy link
Contributor Author

Is there anything else that I can do to help progress this pull request?

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for writing tests! This is good to merge now. The regexp based parsing looks a bit fragile, so having tests is important.

@JukkaL JukkaL merged commit e4b4959 into python:master Aug 14, 2020
@AWhetter AWhetter deleted the fix_stubgen_generic_import branch October 10, 2020 03:26
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.

2 participants