Repository navigation
Improve CLI, refactor and document stubgen - #6256
Conversation
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
This really makes stubgen a much more useful tool! Additional thanks for cleaning up the code and writing docstrings.
This the first part of my review. I'll continue next week.
Some general things:
- Some generated aliases may define a name with an underscore prefix (e.g. if I generate a stub for
os). Should these be filtered out? - Similar to above, some aliases use names with an underscore prefix (e.g. if I generate stub for
os). Should these be declared asx: Anyinstead? - Some generated constants are declared as
x: Final. Should these bex: Final[Any]instead?
|
@JukkaL Thanks for review! I implemented your comments. Regarding private names: it is now controlled by A general comment, I didn't strive for completeness in this PR, small fixes (like above) can be added to this PR, but for all larger things I would rather open follow-up issues. |
|
(AppVeyor failure is a flake) |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for updates! This concludes my review; left a bunch of mostly minor nits. Feel free to ignore things that aren't relevant and create follow-up issues for things that are non-trivial to address.
|
OK, playing with real stub generation discovered an issue: we should not add |
| Don't try to import modules, instead use mypy's normal mechanisms to find | ||
| sources. This will not find any C extension modules. Stubgen also uses | ||
| runtime introspection to find actual value of ``__all__``, so with this flag | ||
| the set of re-expoted names may be incomplete. This flag will be useful if |
|
|
||
| ``--parse-only`` | ||
| Don't perform mypy semantic analysis of source files. This may generate | ||
| worse stubs, in particular some module, class, and function aliases may |
There was a problem hiding this comment.
first comma should be a colon
|
The changes in response to my review look good. |
|
|
||
| class A(metaclass=ABCMeta): | ||
| @abstractmethod | ||
| def meth(self) -> None: ... |
There was a problem hiding this comment.
Should the return type be Any?
There was a problem hiding this comment.
The return is Any only if we run semantic analyzer (test name should end in _semanal), see a test below. But on the second thought it looks like it is not always necessary, in most common cases (like when we add the decorator here) it is better to return Any. I will fix this now.
|
|
||
| class A(metaclass=abc.ABCMeta): | ||
| @abc.abstractmethod | ||
| def meth(self) -> None: ... |
There was a problem hiding this comment.
Again, should the return type be Any?
Also use tighter formatting in tests.
|
Oh, GitHub somehow completely screwed up my merge. I will try to fix it. |
|
OK, it was actually my fault, I didn't add one file to the commit :P |
|
Unless there are some objections, I am going to merge this soon after tests pass. |
3 participants
This PR does several "infrastructure" changes to
stubgentool:mypyCLIstubgen.main()into independent functionsstubdoc.pystubgen.pytostubutil.pyand reorganize the latterDataSuitestubgenscriptThis also does few smaller things:
stubgenc.pyand (new)stubdoc.pyThis is not a pure refactoring, turning the semantic analysis on required some (although relatively small) changes in logic (because the sources should be semantically analyzed as a whole). It also required couple minor changes in
semanal.pyandbuild.py.