-
-
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
Type ignore comments erroneously marked as unused by dmypy #15043
base: master
Are you sure you want to change the base?
Changes from all commits
1ce2f21
7d9c06a
9cd625d
94e5df4
b5cc83f
99eda13
d798c5a
ad94241
22777be
64cbe6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -684,10 +684,11 @@ def refresh_file(module: str, path: str) -> list[str]: | |
|
||
# Find all original modules in graph that were not reached -- they are deleted. | ||
to_delete = [] | ||
seen_and_ancestors = self._seen_and_ancestors(seen) | ||
for module_id in orig_modules: | ||
if module_id not in graph: | ||
continue | ||
if module_id not in seen: | ||
if module_id not in seen_and_ancestors: | ||
module_path = graph[module_id].path | ||
assert module_path is not None | ||
to_delete.append((module_id, module_path)) | ||
|
@@ -715,6 +716,29 @@ def refresh_file(module: str, path: str) -> list[str]: | |
|
||
return messages | ||
|
||
def _seen_and_ancestors(self, seen: set[str]) -> set[str]: | ||
"""Return the set of seen modules along with any ancestors not already in the set. | ||
|
||
For example, given this set: | ||
|
||
{"foo", "foo.bar", "a.b.c"} | ||
|
||
... we would expect this set to be returned: | ||
|
||
{"foo", "foo.bar", "a.b.c", "a.b", "a"} | ||
|
||
This is used to stop us from deleting ancestor modules from the graph | ||
when their descendants have been seen. | ||
""" | ||
seen_paths = seen.copy() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No big deal, but we could do this without any conditionals like this, if you prefer:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed that in some cases with many sub-modules that my way would be faster so my preference would be to keep it as-is, but I'm not strongly opposed to changing this if there's a strong will to do so? |
||
for module_path in seen: | ||
while module_path := module_path.rpartition(".")[0]: | ||
if module_path in seen_paths: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we might be able to drop this conditional and just always add the module_path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like it'd be quicker to skip this if we have a module with many submodules. I haven't measured this though. |
||
break | ||
else: | ||
seen_paths.add(module_path) | ||
return seen_paths | ||
|
||
def find_reachable_changed_modules( | ||
self, | ||
roots: list[BuildSource], | ||
|
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.
Great docstring, thanks.