Modifying multiple files per update in fine-grained incremental mode #4282
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gvanrossum
reviewed
Nov 28, 2017
| assert len(changed_modules) == 1, changed_modules | ||
| changed_modules = get_all_changed_modules(module, path, old_graph, graph) | ||
| # If there are multiple modules to process, only process one of them and return the | ||
| # remaining ones to the caller. |
There was a problem hiding this comment.
Maybe note that you process the last one in the list first? This is significant, because the first one in the list is guaranteed (by the implementation of get_all_changed_modules()) to be the original module.
| for id, _ in remaining_modules: | ||
| del manager.modules[id] | ||
| del graph[id] | ||
| module, path = changed_modules[-1] |
There was a problem hiding this comment.
Somehow this came as a small surprise to me -- I would chop off the designated item first. (Maybe using pop() so the slice on line 262 is unnecessary.)
| # TODO: state.fix_suppressed_dependencies()? | ||
|
|
||
| # Run remaining passes of semantic analysis. | ||
| try: | ||
| state.semantic_analysis() | ||
| except CompileError as err: | ||
| # TODO: What if there are multiple changed modules? |
There was a problem hiding this comment.
That TODO is currently irrelevant.
| for st in new_graph.values(): | ||
| if st.id not in old_graph and st.id not in changed_set: | ||
| changed_set.add(st.id) | ||
| assert st.path |
There was a problem hiding this comment.
The assert looks odd in the middle of updating the two parallel data structures -- I'd put it at the top of the block.
|
I'll add suggested improvements to my next PR if they are still relevant. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The approach is to split multiple file updates into multiple single-file
updates, and only report errors for the final update. If a modified file
brings in additional files through an import, we'll process one of the
imported files instead, changing the target file as we go. The
motivation is that this should result in less redundant work than
postponing the imported file, as a missing imported file will result
in a lot of stale targets which would need to processed again on the
next update.
This still doesn't handle blockers well. I'll fix these issues separately.
This is inefficient as some files can be parsed multiple times, with
O(n**2) worst case behavior (n is the number of changed files). It
should be fine when a small number of files need to be processed.