-
-
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
stubtest script to compare imported module and stub #3036
Conversation
Error defined as NamedTuple(name, error_type, message). stubtest no longer errors when there is no stubfile
Instead of calling the dumpmodule script, do a relative import and call the function directly.
unittest.mock problem related to https://bugs.python.org/issue25532
also changed dump['attrs'] to a dict
doing this automatically depends on module stdlib_list, which is not suitable for master branch.
Please don't squash, it makes review harder. We always squash when we merge.
|
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 like your refactoring, it looks much cleaner now.
BTW, you might want to use a different branch than master
locally for PRs, otherwise if you ever have to update and merge from the official master, it will be impossible.
dump_simple(p.default)) | ||
for name, p in params], | ||
'args': [(name, param_kind(p), dump_simple(p.default)) | ||
for name, p in params], |
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.
No change here, so can keep the original format to reduce diffs?
data_dir = default_data_dir(None) | ||
options = Options() | ||
options.python_version = (3, 6) | ||
lib_path = default_lib_path(data_dir, | ||
options.python_version, | ||
custom_typeshed_dir=None) | ||
sources = find_modules_recursive(id, lib_path) | ||
if not sources: | ||
sys.exit('Error: Cannot find module {}'.format(repr(id))) |
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.
Why is it no longer necessary to check for empty sources?
instance_children = defaultdict(lambda: None, instance['names']) | ||
|
||
# TODO: I would rather not filter public children here. | ||
# For example, what if the checkersurfaces an inconsistency |
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.
space after 'checker'
|
||
# TODO: I would rather not filter public children here. | ||
# For example, what if the checkersurfaces an inconsistency | ||
# in the typing of a private child |
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.
But if private children are not supposed to be used outside the module, is it worth trying to add them to the stub, especially since some of them may be very tricky to type correctly?
@@ -32,16 +33,26 @@ def module_to_json(m): | |||
result = {} | |||
for name, value in m.__dict__.items(): | |||
# Filter out some useless attributes. | |||
|
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.
unnecessary empty line
_, line = inspect.getsourcelines(getattr(m, name)) | ||
except (TypeError, OSError): | ||
line = None | ||
|
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.
unnecessary empty line
line = None | ||
|
||
result[name]['line'] = line | ||
|
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.
unnecessary empty line
|
||
messages = { | ||
'not_in_runtime': ('{error.stub_type} "{error.name}" defined at line ' | ||
' {error.line} in stub but is not defined at runtime'), |
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.
If line is None, you might want to suppress output of at line None
# TODO: can we filter only call? | ||
} | ||
|
||
messages = { |
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 would use constants like NOT_IN_RUNTIME = ('{error.stub_type} ....
, instead of dictionary; that way IDE auto-completes and interpreter will identify a typo right away.
|
||
@verify.register(nodes.TypeVarExpr) | ||
def verify_typevarexpr(node, module_node): | ||
if False: |
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.
Why do you need if False: yield None
here and in several other places? Why not just pass
?
This script doesn't seem to affect anything except dumpmodule, so I'll just merge this now and we'll can iterate later. |
Code from the sprint this weekend.
There are a few things I'd like to finish up
I think I can finish these up in the evening of this week, but the code is here in case you want to run out ahead, or if I get hit by a bus.
Also let me know if you want the commits squashed, or anything else before merging.
-- Jared