Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Clear code coverage upon file save #2586

Closed
wants to merge 2 commits into from
Closed

Clear code coverage upon file save #2586

wants to merge 2 commits into from

Conversation

alanlal
Copy link

@alanlal alanlal commented Jun 20, 2019

PR for #2551

Update goMain.ts

Added a new function to goCover.ts , removeCodeCoverageOnFileSave to handle File Save for clearing code coverage.

@msftclas
Copy link

msftclas commented Jun 20, 2019

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alanlal Looks like this is your first PR contribution to this project, Thanks & Welcome!

Going through it I realized that with this change we lose out a fix we had provided a few months ago where we avoid clearing the code coverage when the changes that are made are inside a comment.

This is because the file change event provides us with actual content that was changed, but the file save event doesnt provide any such information.

Can you look at how we can make use of the file change event information to guide the decision of whether or not to clear the code coverage on file save?

@alanlal
Copy link
Author

alanlal commented Jun 30, 2019

Hi @ramya-rao-a,

I'm having trouble finding a solution to this. Could you help me figure out the areas that I should check out? I looked at the vscode api docs ; but I couldn't find a file save event listener that takes arguments that contains file change information.

kegsay pushed a commit to kegsay/vscode-go that referenced this pull request Oct 22, 2019
This also preserves an earlier fix to keep coverage when the modifications
are only due to comment edits.

Supersedes microsoft#2586
kegsay added a commit to kegsay/vscode-go that referenced this pull request Oct 22, 2019
This also preserves an earlier fix to keep coverage when the modifications
are only due to comment edits.

Supersedes microsoft#2586
kegsay added a commit to kegsay/vscode-go that referenced this pull request Oct 22, 2019
This also preserves an earlier fix to keep coverage when the modifications
are only due to comment edits.

Supersedes microsoft#2586
kegsay added a commit to kegsay/vscode-go that referenced this pull request Oct 22, 2019
This also preserves an earlier fix to keep coverage when the modifications
are only due to comment edits.

Supersedes microsoft#2586
@@ -336,7 +336,7 @@ export function activate(ctx: vscode.ExtensionContext): void {
ctx.subscriptions.push(lintDiagnosticCollection);
vetDiagnosticCollection = vscode.languages.createDiagnosticCollection('go-vet');
ctx.subscriptions.push(vetDiagnosticCollection);
vscode.workspace.onDidChangeTextDocument(removeCodeCoverageOnFileChange, null, ctx.subscriptions);
vscode.workspace.onDidSaveTextDocument(removeCodeCoverageOnFileSave, null, ctx.subscriptions);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this line go into the function addOnSaveTextDocumentListeners

@ramya-rao-a
Copy link
Contributor

@alanlal Looks like there is no good way to get the changes done to a document when it is saved.

@ramya-rao-a
Copy link
Contributor

Closing this PR as there hasnt been any updates to it since last June

@alwindoss
Copy link

@ramya-rao-a I thought someone had raised a PR for the same. If it's not so let me give this a shot.

However, why are we closing this issue, it is not fixed right?

@ramya-rao-a
Copy link
Contributor

@alwindoss, The issue is not being closed, only the PR is being closed. The issue #2551 is still open and I will follow up on the alternate PR #2853 that builds on your work and addresses the comments I had made.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants