-
Notifications
You must be signed in to change notification settings - Fork 646
Add live error feedback as you type #903
Changes from 6 commits
a76f8d2
4335c9f
910cd8d
0f65407
f57bc9c
8ce24b6
4b51004
816b8c4
aaf028a
b6d764c
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 |
---|---|---|
@@ -0,0 +1,100 @@ | ||
'use strict'; | ||
|
||
import vscode = require('vscode'); | ||
import { byteOffsetAt, getBinPath } from './util'; | ||
import cp = require('child_process'); | ||
import path = require('path'); | ||
import { promptForMissingTool } from './goInstallTools'; | ||
import { diagnosticCollection } from './goMain'; | ||
|
||
// Interface for settings configuration for adding and removing tags | ||
interface GoLiveErrorsConfig { | ||
delay: number; | ||
enabled: boolean; | ||
} | ||
|
||
let runner; | ||
|
||
// parseLiveFile runs the gotype command in live mode to check for any syntactic or | ||
// semantic errors and reports them immediately | ||
export function parseLiveFile(e: vscode.TextDocumentChangeEvent) { | ||
if (e.document.isUntitled) { | ||
return; | ||
} | ||
if (e.document.languageId !== 'go') { | ||
return; | ||
} | ||
|
||
let config = <GoLiveErrorsConfig>vscode.workspace.getConfiguration('go')['liveErrors']; | ||
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. why not re-use 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 use 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'm re-using it now, since we are also checking for |
||
if (config == null || !config.enabled) { | ||
return; | ||
} | ||
|
||
if (runner != null) { | ||
clearTimeout(runner); | ||
} | ||
runner = setTimeout(function(){ | ||
processFile(e); | ||
runner = null; | ||
}, config.delay); | ||
} | ||
|
||
export function goLiveErrorsEnabled() { | ||
return <GoLiveErrorsConfig>vscode.workspace.getConfiguration('go')['liveErrors'].enabled; | ||
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. add a null/undefined check for 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. 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? |
||
} | ||
|
||
// processFile does the actual work once the timeout has fired | ||
function processFile(e: vscode.TextDocumentChangeEvent) { | ||
let uri = e.document.uri; | ||
let gotypeLive = getBinPath('gotype-live'); | ||
let fileContents = e.document.getText(); | ||
let fileName = e.document.fileName; | ||
let args = ['-e', '-a', '-lf=' + fileName, path.dirname(fileName)]; | ||
let p = cp.execFile(gotypeLive, args, (err, stdout, stderr) => { | ||
if (err && (<any>err).code === 'ENOENT') { | ||
promptForMissingTool('gotype-live'); | ||
return; | ||
} | ||
|
||
// we want to take the error path here because the command we are calling | ||
// returns a non-zero exit status if the checks fail | ||
let newDiagnostics = []; | ||
if (!newDiagnostics) { | ||
newDiagnostics = []; | ||
} | ||
|
||
// grab a copy of the existing diagnostics that are being reported for the | ||
// current file | ||
let oldDiagnostics = diagnosticCollection.get(uri); | ||
|
||
// delete the existing diagnostics for the current file | ||
// | ||
// 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); | ||
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.
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. 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 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 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 tried using two separate diagnostics, but when I called 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? |
||
|
||
// we want to keep all non-error level diagnostics that were previously | ||
// reported, so add them back in | ||
oldDiagnostics.forEach((value) => { | ||
if (value.severity !== vscode.DiagnosticSeverity.Error) { | ||
newDiagnostics.push(value); | ||
} | ||
}); | ||
|
||
if (err) { | ||
stderr.split('\n').forEach(error => { | ||
if (error === null || error.length === 0) { | ||
return; | ||
} | ||
// extract the line, column and error message from the gotype output | ||
let [_, line, column, message] = /^.+:(\d+):(\d+):\s+(.+)/.exec(error); | ||
|
||
let range = new vscode.Range(+line - 1, +column, +line - 1, +column); | ||
let diagnostic = new vscode.Diagnostic(range, message, vscode.DiagnosticSeverity.Error); | ||
newDiagnostics.push(diagnostic); | ||
}); | ||
} | ||
diagnosticCollection.set(uri, newDiagnostics); | ||
}); | ||
p.stdin.end(fileContents); | ||
} |
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.
copy paste error ? :)
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.
Hmm.. I don't see an error. What am I missing? :)
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.
Oh I meant that the description needs an update :)