-
Notifications
You must be signed in to change notification settings - Fork 645
Added commands "Go Lint Package" and "Go Lint Workspace" #1258
Conversation
7259f77
to
f6936fa
Compare
I don't know why happened this job error, so reopening pull request to trigger travis |
89e20b7
to
3d8a1f0
Compare
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.
@ndrewnee Nice work!
Thanks for your patience, work got real busy this last month and I wasnt able to devote as much time as I'd have liked for reviewing PRs.
I have added 2 comments, 1 is a simple refactor, the other might need some work.
Also, can you rebase or merge from the master branch? There have been quite some changes there.
Thanks!
src/goMain.ts
Outdated
let editor = vscode.window.activeTextEditor; | ||
let documentUri = editor ? editor.document.uri : null; | ||
let goConfig = vscode.workspace.getConfiguration('go', documentUri); | ||
|
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.
let's move all this logic inside lintCurrentPackage
function. This is to be consistent with all the other commands and to keep the goMain.ts
file clear
Same for lintWorkspace
function
src/goMain.ts
Outdated
let goConfig = vscode.workspace.getConfiguration('go', documentUri); | ||
|
||
lintCurrentPackage(documentUri, goConfig) | ||
.then(handleDiagnosticErrors(editor ? editor.document : null)) |
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.
calling handleDiagnosticErrors
clears any existing build and vet errors. It should only update the warnings.
- Cleared goMain.ts, moved logic to lintCurrentPackage and lintWorkspace in goLint.ts - Moved function handleDiagnosticErrors from goMain.ts to util.ts - Fixed tests
@ramya-rao-a No problem) I merged master and made some changes, you can view it. Thanks for your review) |
Merged, thanks @ndrewnee! |
Fixes #1041