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

Add live error feedback as you type #903

Merged
merged 10 commits into from
Apr 2, 2017
Merged

Add live error feedback as you type #903

merged 10 commits into from
Apr 2, 2017

Conversation

tylerstillwater
Copy link
Contributor

@tylerstillwater tylerstillwater commented Mar 31, 2017

This PR adds an optional (disabled by default) live error reporting system.

Closes #883

Motivation

I've used many editors in which a live as-you-type feedback system was available. I've found that it helps me catch errors as they happen and fix them immediately. This constant feedback allows me to code more quickly, as I am still in the context of an error when I see it, rather than having to come back to it after the build has failed.

Implementation

To implement this feature, I have taken advantage of my custom fork of gotype, called gotype-live.

My fork enables the processing of an in-memory version of a file by accepting the contents via stdin. This allows us to get errors back from gotype as we code, rather than having to save the file first.

Configuration

The default configuration for this feature, as of now, is as follows:

"go.liveErrors": {
    "enabled": false,
    "delay": 500
}

The delay value configures a timer that waits for nms after a keystroke before running the process. If a keystroke occurs during that window, the timer is reset.

Example

Below is a short video showing the feature in action. This video was recorded with a 250ms delay time.

feedback

@msftclas
Copy link

@tylerb,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@tylerstillwater
Copy link
Contributor Author

@ramya-rao-a I would greatly appreciate a code review, as I have never worked in typescript before. I want to make sure my code is clean and correct. 😄

@msftclas
Copy link

@tylerb, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@tylerstillwater tylerstillwater changed the title Add live as-you-type error reporting Add live error feedback as you type Mar 31, 2017
package.json Outdated
@@ -541,6 +541,23 @@
},
"description": "Tags and options configured here will be used by the Add Tags command to add tags to struct fields. If promptForTags is true, then user will be prompted for tags and options. By default, json tags are added."
},
"go.liveErrors": {
"enabled": {
Copy link
Contributor

Choose a reason for hiding this comment

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

The "enabled" and "delay" objects here should be inside "properties":{}. Only then will the validation work i.e green squiggly appears when user types anything other than a boolean for "enabled"

@@ -26,7 +26,8 @@ function getTools(goVersion: SemVersion): { [key: string]: string } {
'go-symbols': 'github.com/acroca/go-symbols',
'guru': 'golang.org/x/tools/cmd/guru',
'gorename': 'golang.org/x/tools/cmd/gorename',
'gomodifytags': 'github.com/fatih/gomodifytags'
'gomodifytags': 'github.com/fatih/gomodifytags',
'gotype-live': 'github.com/tylerb/gotype-live'
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be inside an if block so that only people who have opted-in to use the live error feature get the tool installed when Go: Install Tools is run

return;
}

diagnosticCollection.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with clearing the diagnosticCollection is that any diagnostics prior to running the processFile will disappear. For example: all linting and vetting warnings will be removed.

You could create a new collection and all the errors from gotype there, but on save you will end up with multiple errors (1 from gotype the other from go build) showing up in the Problems pane.

What you could then do is check the current collection if it has any errors/warnings for the same line that gotype is returning an error. If yes, then let the current collection win, if not then go ahead and add the error to the new collection.

@ramya-rao-a
Copy link
Contributor

@tylerb Great job! Your typescript is spot on :)

I have a left a few comments, have a look.

@tylerstillwater
Copy link
Contributor Author

@ramya-rao-a

I've made the changes you requested.

Regarding the changes for preserving existing diagnostics, here is the approach I've used:

  1. Load existing diagnostics for this file
  2. Delete existing diagnostics for this file
  3. Iterate through the previous diagnostics and re-add any non-error level diagnostics
  4. Iterate through errors reported by gotype-live, if any, and add them to the array
  5. Set the new diagnostic array into the diagnosticCollection

The goal is to preserve all non-error level diagnostics, because gotype-live should be reporting any error-level diagnostics that we'd see during a build, so we are safe to clear those out. This lets us avoid duplicate reports while preserving diagnostics that already existed.

Please review and let me know if you need anything else.

package.json Outdated
"enabled": false,
"delay": 500
},
"description": "Tags and options configured here will be used to configure how the live errors system behaves."
Copy link
Contributor

Choose a reason for hiding this comment

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

copy paste error ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. I don't see an error. What am I missing? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I meant that the description needs an update :)

return;
}

let config = <GoLiveErrorsConfig>vscode.workspace.getConfiguration('go')['liveErrors'];
Copy link
Contributor

Choose a reason for hiding this comment

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

why not re-use goLiveErrorsEnabled() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use config.delay a few lines below, otherwise I would have reused that function, yeah.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm re-using it now, since we are also checking for autoSave.

}

export function goLiveErrorsEnabled() {
return <GoLiveErrorsConfig>vscode.workspace.getConfiguration('go')['liveErrors'].enabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

add a null/undefined check for vscode.workspace.getConfiguration('go')['liveErrors'] before accessing the enabled property

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think that when auto save is enabled, we shouldn't be enabling live errors. That would just be redundant work being done by build and gotype.

Can you add that check here?

//
// error-level diagnostics will be reported by this process, so we want to
// clear out the existing errors to avoid getting duplicates
diagnosticCollection.delete(uri);
Copy link
Contributor

Choose a reason for hiding this comment

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

gotype-live should be reporting any error-level diagnostics that we'd see during a build

If the above assumption is indeed true, I'd still suggest having 2 separate diagnostic collections.

One for warnings (this will be fed by the linting and vetting), the other for errors (this will be fed by build and live errors).

This way you don't have to copy the warnings from currentDiagnostics.
The rest of your code here (delete diagnostics for uri etc.) can remain

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not too picky about this one though. Perf wise, it shouldn't be a problem because you are only touching diagnostics for a single file. But it just feels wrong (gut wise) to be copying array items everytime liveError is run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using two separate diagnostics, but when I called .set on the live diagnostic object, it erased the warning diagnostics that were there. It appeared to me that you can only have one diagnostic object in use, per file, at a time.

I may have done something wrong, though. Is it possible for me to use two separate diagnostic objects on the same file and the same time and the diagnostics from both objects will be displayed?

@ramya-rao-a
Copy link
Contributor

This is great work @tylerb!

I just added 1 commit with some description update, edit to README etc.

Will merge as soon as the tests are green.

@tylerstillwater
Copy link
Contributor Author

tylerstillwater commented Apr 2, 2017 via email

@tylerstillwater
Copy link
Contributor Author

Looks like the gometalinter test failed. I can't retry, though, it seems.

@ramya-rao-a ramya-rao-a merged commit 6db72e6 into microsoft:master Apr 2, 2017
return false;
}
let autoSave = vscode.workspace.getConfiguration('files')['autoSave'];
if (autoSave !== null && autoSave !== undefined && autoSave !== 'off') {
Copy link

Choose a reason for hiding this comment

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

I know it is closed but to be honest, I don't see a reason why autosave is checked at all. I use autosave onFocusChange and why would it be clashing with a linter running after some delay? Even autosave afterDelay makes little sense to check as it may just be some much higher value than linter delay.

Copy link
Contributor

@ramya-rao-a ramya-rao-a Apr 4, 2017

Choose a reason for hiding this comment

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

I was erring on the side of caution.

Good point on the fact that auto save onFocusChange and onWindowChange has no affect whatsoever.

I was concerned with the autosave on delay. Then you have build and gotype both running and giving essentially similar results and overriding each other.

On second thoughts, it is definitely being over cautious. We can remove the check in the next update. And leave it up to the user to choose between the 2 features if the experience is not pleasant

Copy link

Choose a reason for hiding this comment

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

Thanks!

@keithballdotnet
Copy link

@tylerb just wanted to come along and say that this is a great feature. Love it!!! Thank you!!

@tylerstillwater
Copy link
Contributor Author

@keithballdotnet glad you're enjoying it! You're most welcome!

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.

Feedback as you type
6 participants