Skip to content

Commit f034b22

Browse files
h9jianggopherbot
authored andcommitted
extension/src/diagnostics: refactor handle errors function
- The optional source input parameter is not being referenced. - Some variable names are too long. - Replace forEach function with for loop: https://google.github.io/styleguide/tsguide.html#control-flow-statements-blocks - Indent code generate value for some variable for readability. - Inline function mapSeverityToVSCodeSeverity. Change-Id: Ie18dbfd8f33c81e35d8b8cfbed5f4f963625a9b5 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/808280 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Hongxiang Jiang <hxjiang@golang.org> Reviewed-by: Peter Weinberger <pjw@google.com>
1 parent 204b57a commit f034b22

7 files changed

Lines changed: 89 additions & 91 deletions

File tree

extension/src/commands/runBuilds.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import * as vscode from 'vscode';
77

88
import { check } from '../diagnostics/goCheck';
99
import { CommandFactory } from '.';
10-
import { handleDiagnosticErrors } from '../diagnostics/diagnostics';
10+
import { handleErrors } from '../diagnostics/diagnostics';
1111

1212
export const runBuilds: CommandFactory =
1313
(ctx, goCtx) => (document: vscode.TextDocument, goConfig: vscode.WorkspaceConfiguration) => {
@@ -21,9 +21,9 @@ export const runBuilds: CommandFactory =
2121
vetDiagnosticCollection?.clear();
2222
check(goCtx, document.uri, goConfig)
2323
.then((results) => {
24-
results.forEach((result) => {
25-
handleDiagnosticErrors(goCtx, document, result.errors, result.diagnosticCollection);
26-
});
24+
for (const result of results) {
25+
handleErrors(goCtx, document, result.errors, result.diagnosticCollection);
26+
}
2727
})
2828
.catch((err) => {
2929
vscode.window.showInformationMessage('Error: ' + err);

extension/src/diagnostics/diagnostics.ts

Lines changed: 76 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -14,86 +14,100 @@ export interface ICheckResult {
1414
severity: string;
1515
}
1616

17-
export function handleDiagnosticErrors(
17+
export function handleErrors(
1818
goCtx: GoExtensionContext,
1919
document: vscode.TextDocument | undefined,
2020
errors: ICheckResult[],
21-
diagnosticCollection?: vscode.DiagnosticCollection,
22-
diagnosticSource?: string
21+
collection?: vscode.DiagnosticCollection
2322
) {
24-
diagnosticCollection?.clear();
25-
26-
const diagnosticMap: Map<string, vscode.Diagnostic[]> = new Map();
23+
const docMap: Map<string, vscode.TextDocument> = new Map();
24+
{
25+
if (document) {
26+
docMap.set(document.uri.toString(), document);
27+
}
2728

28-
const textDocumentMap: Map<string, vscode.TextDocument> = new Map();
29-
if (document) {
30-
textDocumentMap.set(document.uri.toString(), document);
31-
}
32-
// Also add other open .go files known to vscode for fast lookup.
33-
vscode.workspace.textDocuments.forEach((t) => {
34-
const fileName = t.uri.toString();
35-
if (!fileName.endsWith('.go')) {
36-
return;
29+
// Also add other open .go files known to vscode for fast lookup.
30+
for (const doc of vscode.workspace.textDocuments) {
31+
const fileName = doc.uri.toString();
32+
if (!fileName.endsWith('.go')) {
33+
continue;
34+
}
35+
docMap.set(fileName, doc);
3736
}
38-
textDocumentMap.set(fileName, t);
39-
});
37+
}
38+
39+
const diagsMap: Map<string, vscode.Diagnostic[]> = new Map();
40+
for (const error of errors) {
41+
const uri = vscode.Uri.file(error.file).toString();
4042

41-
errors.forEach((error) => {
42-
const canonicalFile = vscode.Uri.file(error.file).toString();
43-
let startColumn = error.col ? error.col - 1 : 0;
44-
let endColumn = startColumn + 1;
4543
// Some tools output only the line number or the start position.
4644
// If the file content is available, adjust the diagnostic range so
4745
// the squiggly underline for the error message is more visible.
48-
const doc = textDocumentMap.get(canonicalFile);
49-
if (doc) {
50-
const tempRange = new vscode.Range(
51-
error.line - 1,
52-
0,
53-
error.line - 1,
54-
doc.lineAt(error.line - 1).range.end.character + 1 // end of the line
55-
);
56-
const text = doc.getText(tempRange);
57-
const [, leading, trailing] = /^(\s*).*(\s*)$/.exec(text)!;
58-
if (!error.col) {
59-
startColumn = leading.length; // beginning of the non-white space.
60-
} else {
61-
startColumn = error.col - 1; // range is 0-indexed
46+
let range: vscode.Range;
47+
{
48+
let startColumn = error.col ? error.col - 1 : 0;
49+
let endColumn = startColumn + 1;
50+
const doc = docMap.get(uri);
51+
if (doc) {
52+
const tempRange = new vscode.Range(
53+
error.line - 1,
54+
0,
55+
error.line - 1,
56+
doc.lineAt(error.line - 1).range.end.character + 1 // end of the line
57+
);
58+
const text = doc.getText(tempRange);
59+
const [, leading, trailing] = /^(\s*).*(\s*)$/.exec(text)!;
60+
if (!error.col) {
61+
startColumn = leading.length; // beginning of the non-white space.
62+
} else {
63+
startColumn = error.col - 1; // range is 0-indexed
64+
}
65+
endColumn = text.length - trailing.length;
6266
}
63-
endColumn = text.length - trailing.length;
67+
68+
range = new vscode.Range(error.line - 1, startColumn, error.line - 1, endColumn);
6469
}
65-
const range = new vscode.Range(error.line - 1, startColumn, error.line - 1, endColumn);
66-
const severity = mapSeverityToVSCodeSeverity(error.severity);
67-
const diagnostic = new vscode.Diagnostic(range, error.msg, severity);
68-
// vscode uses source for deduping diagnostics.
69-
diagnostic.source = diagnosticSource || diagnosticCollection?.name;
70-
let diagnostics = diagnosticMap.get(canonicalFile);
71-
if (!diagnostics) {
72-
diagnostics = [];
70+
71+
let severity: vscode.DiagnosticSeverity = vscode.DiagnosticSeverity.Error;
72+
if (error.severity === 'warning') {
73+
severity = vscode.DiagnosticSeverity.Warning;
7374
}
74-
diagnostics.push(diagnostic);
75-
diagnosticMap.set(canonicalFile, diagnostics);
76-
});
7775

78-
diagnosticMap.forEach((newDiagnostics, file) => {
79-
const fileUri = vscode.Uri.parse(file);
76+
const diag = new vscode.Diagnostic(range, error.msg, severity);
77+
diag.source = collection?.name; // vscode uses source for deduping diagnostics.
78+
79+
let diags = diagsMap.get(uri);
80+
if (!diags) {
81+
diags = [];
82+
}
83+
diags.push(diag);
84+
85+
diagsMap.set(uri, diags);
86+
}
87+
88+
collection?.clear();
89+
for (const [uriStr, fileDiags] of diagsMap) {
90+
let diags = fileDiags;
91+
const uri = vscode.Uri.parse(uriStr);
8092

8193
const { buildDiagnosticCollection, lintDiagnosticCollection, vetDiagnosticCollection, languageClient } = goCtx;
82-
if (diagnosticCollection === buildDiagnosticCollection) {
83-
// If there are lint/vet warnings on current file, remove the ones co-inciding with the new build errors
84-
removeDuplicateDiagnostics(lintDiagnosticCollection, fileUri, newDiagnostics);
85-
removeDuplicateDiagnostics(vetDiagnosticCollection, fileUri, newDiagnostics);
86-
} else if (buildDiagnosticCollection && buildDiagnosticCollection.has(fileUri)) {
87-
// If there are build errors on current file, ignore the new lint/vet warnings co-inciding with them
88-
newDiagnostics = deDupeDiagnostics(buildDiagnosticCollection.get(fileUri)!.slice(), newDiagnostics);
94+
if (collection === buildDiagnosticCollection) {
95+
// If there are lint/vet warnings on current file, remove the ones
96+
// co-inciding with the new build errors.
97+
removeDuplicateDiagnostics(lintDiagnosticCollection, uri, diags);
98+
removeDuplicateDiagnostics(vetDiagnosticCollection, uri, diags);
99+
} else if (buildDiagnosticCollection && buildDiagnosticCollection.has(uri)) {
100+
// If there are build errors on current file, ignore the new lint/vet
101+
// warnings co-inciding with them.
102+
diags = deDupeDiagnostics(buildDiagnosticCollection.get(uri)!.slice(), diags);
89103
}
90-
// If there are errors from the language client that are on the current file, ignore the warnings co-inciding
91-
// with them.
92-
if (languageClient && languageClient.diagnostics?.has(fileUri)) {
93-
newDiagnostics = deDupeDiagnostics(languageClient.diagnostics.get(fileUri)!.slice(), newDiagnostics);
104+
// If there are errors from the language client that are on the current file,
105+
// ignore the warnings co-inciding with them.
106+
if (languageClient && languageClient.diagnostics?.has(uri)) {
107+
diags = deDupeDiagnostics(languageClient.diagnostics.get(uri)!.slice(), diags);
94108
}
95-
diagnosticCollection?.set(fileUri, newDiagnostics);
96-
});
109+
collection?.set(uri, diags);
110+
}
97111
}
98112

99113
/**
@@ -121,14 +135,3 @@ function deDupeDiagnostics(
121135
const buildDiagnosticsLines = buildDiagnostics.map((x) => x.range.start.line);
122136
return otherDiagnostics.filter((x) => buildDiagnosticsLines.indexOf(x.range.start.line) === -1);
123137
}
124-
125-
function mapSeverityToVSCodeSeverity(sev: string): vscode.DiagnosticSeverity {
126-
switch (sev) {
127-
case 'error':
128-
return vscode.DiagnosticSeverity.Error;
129-
case 'warning':
130-
return vscode.DiagnosticSeverity.Warning;
131-
default:
132-
return vscode.DiagnosticSeverity.Error;
133-
}
134-
}

extension/src/diagnostics/goBuild.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { diagnosticsStatusBarItem, outputChannel } from '../goStatus';
1414
import { getTestFlags } from '../testUtils';
1515
import { getCurrentGoPath, getModuleCache, getTempFilePath, getWorkspaceFolderPath, runTool } from '../util';
1616
import { getCurrentGoWorkspaceFromGOPATH } from '../utils/pathUtils';
17-
import { handleDiagnosticErrors, ICheckResult } from './diagnostics';
17+
import { handleErrors, ICheckResult } from './diagnostics';
1818

1919
/**
2020
* Builds current package or workspace.
@@ -44,7 +44,7 @@ export function buildCode(buildWorkspace?: boolean): CommandFactory {
4444
isModSupported(documentUri).then((isMod) => {
4545
goBuild(documentUri, isMod, goConfig, buildWorkspace)
4646
.then((errors) => {
47-
handleDiagnosticErrors(goCtx, editor?.document, errors, goCtx.buildDiagnosticCollection);
47+
handleErrors(goCtx, editor?.document, errors, goCtx.buildDiagnosticCollection);
4848
diagnosticsStatusBarItem.hide();
4949
})
5050
.catch((err) => {

extension/src/diagnostics/goLint.ts

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { toolExecutionEnvironment } from '../goEnv';
1111
import { diagnosticsStatusBarItem, outputChannel } from '../goStatus';
1212
import { inspectGoToolVersion } from '../goInstallTools';
1313
import { getBinPath, getWorkspaceFolderPath, resolvePath, runTool } from '../util';
14-
import { handleDiagnosticErrors, ICheckResult } from './diagnostics';
14+
import { handleErrors, ICheckResult } from './diagnostics';
1515

1616
/**
1717
* Runs linter on the current file, package or workspace.
@@ -41,12 +41,7 @@ export function lintCode(scope?: string): CommandFactory {
4141

4242
goLint(documentUri, goConfig, scope)
4343
.then((warnings) => {
44-
handleDiagnosticErrors(
45-
goCtx,
46-
editor ? editor.document : undefined,
47-
warnings,
48-
goCtx.lintDiagnosticCollection
49-
);
44+
handleErrors(goCtx, editor ? editor.document : undefined, warnings, goCtx.lintDiagnosticCollection);
5045
diagnosticsStatusBarItem.hide();
5146
})
5247
.catch((err) => {

extension/src/diagnostics/goVet.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { getGoConfig } from '../config';
1010
import { toolExecutionEnvironment } from '../goEnv';
1111
import { diagnosticsStatusBarItem, outputChannel } from '../goStatus';
1212
import { getGoVersion, getWorkspaceFolderPath, resolvePath, runTool } from '../util';
13-
import { handleDiagnosticErrors, ICheckResult } from './diagnostics';
13+
import { handleErrors, ICheckResult } from './diagnostics';
1414

1515
/**
1616
* Runs go vet in the current package or workspace.
@@ -38,7 +38,7 @@ export function vetCode(vetWorkspace?: boolean): CommandFactory {
3838

3939
goVet(documentUri, goConfig, vetWorkspace)
4040
.then((warnings) => {
41-
handleDiagnosticErrors(goCtx, editor?.document, warnings, goCtx.vetDiagnosticCollection);
41+
handleErrors(goCtx, editor?.document, warnings, goCtx.vetDiagnosticCollection);
4242
diagnosticsStatusBarItem.hide();
4343
})
4444
.catch((err) => {

extension/src/util.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import {
2727
} from './utils/pathUtils';
2828
import { killProcessTree } from './utils/processUtils';
2929
import { ICheckResult } from './diagnostics/diagnostics';
30-
export { ICheckResult, handleDiagnosticErrors, removeDuplicateDiagnostics } from './diagnostics/diagnostics';
30+
export { ICheckResult, handleErrors, removeDuplicateDiagnostics } from './diagnostics/diagnostics';
3131

3232
export class GoVersion {
3333
public sv?: semver.SemVer;

extension/test/integration/linting.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import * as vscode from 'vscode';
1313
import { getGoConfig } from '../../src/config';
1414
import { updateGoVarsFromConfig } from '../../src/goInstallTools';
1515
import { goLint } from '../../src/diagnostics/goLint';
16-
import { handleDiagnosticErrors } from '../../src/util';
16+
import { handleErrors } from '../../src/diagnostics/diagnostics';
1717
import os = require('os');
1818
import { MockWorkspaceConfiguration } from './mocks/configuration';
1919

@@ -105,7 +105,7 @@ suite('Linting', function () {
105105
const warnings = await goLint(file2.uri, config, 'package');
106106

107107
const diagnosticCollection = vscode.languages.createDiagnosticCollection('linttest');
108-
handleDiagnosticErrors({}, file2, warnings, diagnosticCollection);
108+
handleErrors({}, file2, warnings, diagnosticCollection);
109109

110110
// The first diagnostic message for each file should be about the use of MixedCaps in package name.
111111
// Both files belong to the same package name, and we want them to be identical.

0 commit comments

Comments
 (0)