-
-
Notifications
You must be signed in to change notification settings - Fork 433
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
Add purge_files method to CoverageData + unit tests for it #1547
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
511bea7
Add purge_files method to CoverageData, to allow for selective remova…
sdeibel da1b6b9
Fix assert syntax so it's not true; this code isn't reached in the te…
sdeibel 249d4e3
Remove trailing whitespace; did not expect this would matter on a bla…
sdeibel 91f8d80
Add type annotations required by mypy
sdeibel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
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.
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.
After I merged, I noticed this line: if you are only removing one context for the file, then we don't want to remove the file from the file_map, correct? Why is it important to remove it from the map, and what should we do if it's only a single context being removed?
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.
Good catch. It looks like in Wing i'm only calling this with context==None and in all cases I immediately set updated data with set_context and add_lines so all the contexts are restored, but on the right new line numbers. Should I go ahead and just remove the context arg? I think it was a mistake that came from trying to follow what other API calls support for arguments. Now I can't really think of a reason to have that argument.
I do remember popping from _file_map was needed or some aspect of the update did not work because measured_files was called. I'm going to investigate that further since popping here may indeed be the wrong way to solve that part of it.
Another option would be to replace this with a call that purges/replaces data with a new set of lines all in one operation. My initial idea was the low-level approach was a better match for this API, but I could have been wrong.
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.
It looks like the original
cur.execute
call didn't actually remove data because the cursor wasn't iterated. I replaced it withcur.execute_void
and it had a better effect.If you don't need the per-context operation, then let's definitely simplify it to only remove the entire file. I think I'd have it remove the data for the file, but leave the file in the set of measured files, and state that it's possible for a "measured file" to have no data. Would that work for you?
If we find a need for per-context purging later, we could add it.
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've implemented this in #1551 - wasn't sure how to add to this pull request so ended up with a new one. Hope that is OK...