-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Various improvements to stubgen #7921
Conversation
These happen with some frequency in third-party libraries.
We just arbitrarily assume that this means that the module is not a C module. This happens with paste.
Show the module which we failed to import and hints about what to do next.
There used to be a bogus "Nothing to do" message from `mypy.build`.
Add `self` arguments and special casing for many additional dunder methods.
This may also prevent duplicate definitions from being generated. However, some definitions that are needed on certain platforms only or on Python 2 only, for example, may be omitted. This is arguably less bad than crashing.
Also refactor things a little to make it easier to make the change.
Also support class named Optional.
Since these are aliases, this is generally okay, and reduces duplication.
It doesn't seem useful and easily gets stale if the stub is modified.
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.
Looks great! Does this need any doc updates, or are they will be in the second PR?
@@ -262,6 +262,8 @@ def __init__(self) -> None: | |||
self.cache_map = {} # type: Dict[str, Tuple[str, str]] | |||
# Don't properly free objects on exit, just kill the current process. | |||
self.fast_exit = False | |||
# Used to transform source code before parsing if not None | |||
self.transform_source = None # type: Optional[Callable[[Any], Any]] |
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.
Maybe add a TODO to make this more precise when the mypy issue is fixed?
mypy/parse.py
Outdated
if isinstance(source, str): # Work around mypy issue | ||
source = options.transform_source(source) | ||
else: | ||
source = options.transform_source(source) |
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.
Is this because of #1533? It looks like this should not be needed with the Any
above.
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.
Yeah, it's caused by that issue, and this is not actually needed.
|
||
def add_file(self, path: str, result: List[str]) -> None: | ||
options = parse_options(flag_list + extra) | ||
if '--verbose' not in flag_list: |
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.
Maybe it makes sense to add a test for this flag?
assert not is_test_module('foo.bar') | ||
|
||
# The following could be test modules, but we are very conservative and | ||
# don't treat them as such since they could plausibly be real modules. |
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.
👍
|
||
# Avoid some file names that are unnecessary or likely to cause trouble (\n for end of path). | ||
BLACKLIST = [ | ||
'/six.py\n', # Likely vendored six; too dynamic for us to handle |
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.
Maybe make these regexps and use $
instead of \n
?
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'll deal with this in a follow-up PR, since the change would cause a merge conflict.
(name == short or | ||
self.import_tracker.reverse_alias.get(name) == short)) | ||
|
||
def process_member_expr_decorator(self, expr: MemberExpr, context: Decorator) -> bool: |
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.
Maybe add a short docstring for this function (and maybe also process_name_expr_decorator
)?
@@ -1043,8 +1275,6 @@ def generate_stub_from_ast(mod: StubSource, | |||
if subdir and not os.path.isdir(subdir): | |||
os.makedirs(subdir) | |||
with open(target, 'w') as file: | |||
if add_header: | |||
write_header(file, mod.module, pyversion=pyversion) |
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.
Maybe we can keep this as opt-in, but make it more informative, like add a creation date, mypy version, etc.?
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.
TBH, I don't think that the header is useful, since it will likely get stale once the stub is updated.
@JukkaL Since |
A good idea! Can you create an issue to track this? The change itself should be pretty simple. |
This improves various things in stubgen. Here are some notable changes:
these generate parse errors, and stubgen can't recover from them.
out of date and seems redundant, especially for packages with hundreds of
submodules.
--verbose
and--quiet
flags.modules and tests.
@abstractproperty
.typing.Any
and a class namedAny
(etc.).six
to refer to normalsix
.(Even though this is a big PR, most of changes are small and hopefully
this won't be too difficult to review.)