Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NEW (Extension) @W-13992309@ Remove duplicate suppress violations prompts on the same line #92

Merged
merged 3 commits into from
Jun 6, 2024
Merged

NEW (Extension) @W-13992309@ Remove duplicate suppress violations prompts on the same line #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f6f254
NEW (Extension) @W-13992309@ Remove duplicate suppress violations pro…
jag-j Jun 5, 2024
1279b03
NEW (Extension) @W-13992309@ Fix tests
jag-j Jun 5, 2024
5a28102
NEW (Extension) @W-13992309@ add comments
jag-j Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ export async function activate(context: vscode.ExtensionContext): Promise<vscode
outputChannel
});
});
const removeSingleDiagnostic = vscode.commands.registerCommand(Constants.COMMAND_REMOVE_SINGLE_DIAGNOSTIC, (uri: vscode.Uri, diagnostic: vscode.Diagnostic) => {
_removeSingleDiagnostic(uri, diagnostic, diagnosticCollection);
const removeDiagnosticsInRange = vscode.commands.registerCommand(Constants.COMMAND_DIAGNOSTICS_IN_RANGE, (uri: vscode.Uri, range: vscode.Range) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For a better user experience, we should remove all diagnostics on a given line (or even a set of lines). That's because "// NOPMD" is generic and not specific to a rule. So, a line could have both apexdoc and unused var error and both should go away with the "// NOPMD" tag. This wasn't specifically called out in the acceptance criteria or anywhere else, but I think this is the right thing to do.

_removeDiagnosticsInRange(uri, range, diagnosticCollection);
});
outputChannel.appendLine(`Registered command as part of sfdx-code-analyzer-vscode activation.`);
registerScanOnSave(outputChannel);
Expand Down Expand Up @@ -127,18 +127,22 @@ export async function activate(context: vscode.ExtensionContext): Promise<vscode
});
}
});
context.subscriptions.push(runOnActiveFile, runOnSelected, runDfaOnSelectedMethod, removeDiagnosticsOnActiveFile, removeDiagnosticsOnSelectedFile, removeSingleDiagnostic);
context.subscriptions.push(runOnActiveFile, runOnSelected, runDfaOnSelectedMethod, removeDiagnosticsOnActiveFile, removeDiagnosticsOnSelectedFile, removeDiagnosticsInRange);
TelemetryService.sendExtensionActivationEvent(extensionHrStart);
outputChannel.appendLine(`Extension sfdx-code-analyzer-vscode activated.`);
return Promise.resolve(context);
}

export function _removeSingleDiagnostic(uri: vscode.Uri, diagnosticToRemove: vscode.Diagnostic, diagnosticCollection: vscode.DiagnosticCollection) {
export function _removeDiagnosticsInRange(uri: vscode.Uri, range: vscode.Range, diagnosticCollection: vscode.DiagnosticCollection) {
const currentDiagnostics = diagnosticCollection.get(uri) || [];
const updatedDiagnostics = currentDiagnostics.filter(diagnostic => diagnostic != diagnosticToRemove);
const updatedDiagnostics = filterOutDiagnosticsInRange(currentDiagnostics, range);
diagnosticCollection.set(uri, updatedDiagnostics);
}

function filterOutDiagnosticsInRange(currentDiagnostics: readonly vscode.Diagnostic[], range: vscode.Range) {
return currentDiagnostics.filter(diagnostic => (diagnostic.range.start.line != range.start.line && diagnostic.range.end.line != range.end.line));
}

export async function _stopExistingDfaRun(context: vscode.ExtensionContext, outputChannel: vscode.LogOutputChannel): Promise<void> {
const pid = context.workspaceState.get(Constants.WORKSPACE_DFA_PROCESS);
if (pid) {
Expand Down
2 changes: 1 addition & 1 deletion src/lib/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export const COMMAND_RUN_ON_SELECTED = 'sfca.runOnSelected';
export const COMMAND_RUN_DFA_ON_SELECTED_METHOD = 'sfca.runDfaOnSelectedMethod';
export const COMMAND_REMOVE_DIAGNOSTICS_ON_ACTIVE_FILE = 'sfca.removeDiagnosticsOnActiveFile';
export const COMMAND_REMOVE_DIAGNOSTRICS_ON_SELECTED_FILE = 'sfca.removeDiagnosticsOnSelectedFile';
export const COMMAND_REMOVE_SINGLE_DIAGNOSTIC = 'sfca.removeSingleDiagnostic'
export const COMMAND_DIAGNOSTICS_IN_RANGE = 'sfca.removeDiagnosticsInRange'


// telemetry event keys
Expand Down
22 changes: 15 additions & 7 deletions src/lib/fixer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ export class Fixer implements vscode.CodeActionProvider {
* @returns All actions corresponding to diagnostics in the specified range of the target document.
*/
public provideCodeActions(document: vscode.TextDocument, range: vscode.Range, context: vscode.CodeActionContext): vscode.CodeAction[] {
const processedLines = new Set<number>();
// Iterate over all diagnostics.
return context.diagnostics
// Throw out diagnostics that aren't ours, or are for the wrong line.
.filter(diagnostic => messages.diagnostics.source && messages.diagnostics.source.isSource(diagnostic.source) && diagnostic.range.isEqual(range))
// Get and use the appropriate fix generator.
.map(diagnostic => this.getFixGenerator(document, diagnostic).generateFixes())
.map(diagnostic => this.getFixGenerator(document, diagnostic).generateFixes(processedLines))
// Combine all the fixes into one array.
.reduce((acc, next) => [...acc, ...next], []);
}
Expand Down Expand Up @@ -72,15 +73,16 @@ abstract class FixGenerator {
* Abstract template method for generating fixes.
* @abstract
*/
public abstract generateFixes(): vscode.CodeAction[];
public abstract generateFixes(processedLines: Set<number>): vscode.CodeAction[];
}

/**
* FixGenerator to be used by default when no FixGenerator exists for a given engine. Does nothing.
* @private Must be exported for testing purposes, but shouldn't be used publicly, hence the leading underscore.
*/
export class _NoOpFixGenerator extends FixGenerator {
public generateFixes(): vscode.CodeAction[] {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
public generateFixes(processedLines: Set<number>): vscode.CodeAction[] {
return [];
}
}
Expand All @@ -94,10 +96,16 @@ export class _PmdFixGenerator extends FixGenerator {
* Generate an array of fixes, if possible.
* @returns
*/
public generateFixes(): vscode.CodeAction[] {
public generateFixes(processedLines: Set<number>): vscode.CodeAction[] {
const fixes: vscode.CodeAction[] = [];
if (this.documentSupportsLineLevelSuppression()) {
fixes.push(this.generateLineLevelSuppression());
// We only check for the start line and not the entire range because irrespective of the range of a specific violation,
// we add the NOPMD tag only on the first line of the violation.
const lineNumber = this.diagnostic.range.start.line;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the main change. Even if we have multiple violations on a single line, we show only one "suppress violation" and that would add the "// NOPMD" tag.

if (!processedLines.has(lineNumber)) {
fixes.push(this.generateLineLevelSuppression());
processedLines.add(lineNumber);
}
}
return fixes;
}
Expand Down Expand Up @@ -127,9 +135,9 @@ export class _PmdFixGenerator extends FixGenerator {
action.edit.insert(this.document.uri, endOfLine, " // NOPMD");
action.diagnostics = [this.diagnostic];
action.command = {
command: Constants.COMMAND_REMOVE_SINGLE_DIAGNOSTIC,
command: Constants.COMMAND_DIAGNOSTICS_IN_RANGE,
title: 'Clear Single Diagnostic',
arguments: [this.document.uri, this.diagnostic]
arguments: [this.document.uri, this.diagnostic.range]
};
return action;
}
Expand Down
8 changes: 4 additions & 4 deletions src/test/suite/extension.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {expect} from 'chai';
import path = require('path');
import {SfCli} from '../../lib/sf-cli';
import Sinon = require('sinon');
import { _runAndDisplayPathless, _runAndDisplayDfa, _clearDiagnostics, _shouldProceedWithDfaRun, _stopExistingDfaRun, _isValidFileForAnalysis, verifyPluginInstallation, _clearDiagnosticsForSelectedFiles, _removeSingleDiagnostic, RunInfo } from '../../extension';
import { _runAndDisplayPathless, _runAndDisplayDfa, _clearDiagnostics, _shouldProceedWithDfaRun, _stopExistingDfaRun, _isValidFileForAnalysis, verifyPluginInstallation, _clearDiagnosticsForSelectedFiles, _removeDiagnosticsInRange, RunInfo } from '../../extension';
import {messages} from '../../lib/messages';
import {TelemetryService} from '../../lib/core-extension-service';
import * as Constants from '../../lib/constants';
Expand Down Expand Up @@ -559,7 +559,7 @@ suite('Extension Test Suite', () => {
expect(diagnosticCollection.get(uri)).to.have.lengthOf(2, 'Expected two diagnostics to be present before removal');

// ===== TEST =====
_removeSingleDiagnostic(uri, diagnosticToRemove, diagnosticCollection);
_removeDiagnosticsInRange(uri, diagnosticToRemove.range, diagnosticCollection);

// ===== ASSERTIONS =====
const remainingDiagnostics = diagnosticCollection.get(uri);
Expand All @@ -575,7 +575,7 @@ suite('Extension Test Suite', () => {
expect(diagnosticCollection.get(uri)).to.be.empty;

// ===== TEST =====
_removeSingleDiagnostic(uri, diagnosticToRemove, diagnosticCollection);
_removeDiagnosticsInRange(uri, diagnosticToRemove.range, diagnosticCollection);

// ===== ASSERTIONS =====
const remainingDiagnostics = diagnosticCollection.get(uri);
Expand All @@ -594,7 +594,7 @@ suite('Extension Test Suite', () => {
expect(diagnosticCollection.get(uri)).to.have.lengthOf(1, 'Expected one diagnostic to be present before attempting removal');

// ===== TEST =====
_removeSingleDiagnostic(uri, diagnosticToRemove, diagnosticCollection);
_removeDiagnosticsInRange(uri, diagnosticToRemove.range, diagnosticCollection);

// ===== ASSERTIONS =====
const remainingDiagnostics = diagnosticCollection.get(uri);
Expand Down
30 changes: 27 additions & 3 deletions src/test/suite/fixer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@ suite('fixer.ts', () => {
});

suite('_NoOpFixGenerator', () => {
const existingFixes = new Set<number>();
suite('#generateFixes()', () => {
test('Returns empty array', () => {
// Doesn't matter what we feed to the no-op constructor.
const fixGenerator = new _NoOpFixGenerator(null, null);

// Attempt to generate fixes.
const fixes: vscode.CodeAction[] = fixGenerator.generateFixes();
const fixes: vscode.CodeAction[] = fixGenerator.generateFixes(existingFixes);

// Verify array is empty.
expect(fixes).to.have.lengthOf(0, 'Should be no fixes');
Expand Down Expand Up @@ -59,7 +60,7 @@ suite('fixer.ts', () => {
const fixGenerator: _PmdFixGenerator = new _PmdFixGenerator(doc, diag);

// Attempt to generate fixes for the file.
const fixes: vscode.CodeAction[] = fixGenerator.generateFixes();
const fixes: vscode.CodeAction[] = fixGenerator.generateFixes(null);

// We should get none.
expect(fixes).to.have.lengthOf(0, 'No fixes should be offered');
Expand All @@ -79,6 +80,7 @@ suite('fixer.ts', () => {
});

suite('Line-level suppression', () => {
const existingFixes = new Set<number>();
test('Appends suppression to end of commentless line', async () => {
// Create our fake diagnostic, positioned at the line with no comment at the end.
const diag = new vscode.Diagnostic(
Expand All @@ -94,7 +96,7 @@ suite('fixer.ts', () => {
const fixGenerator: _PmdFixGenerator = new _PmdFixGenerator(doc, diag);

// Attempt to generate fixes for the file.
const fixes: vscode.CodeAction[] = fixGenerator.generateFixes();
const fixes: vscode.CodeAction[] = fixGenerator.generateFixes(existingFixes);

// We expect to get one fix, to inject the suppression at the end of the line.
expect(fixes).to.have.lengthOf(1, 'Wrong action count');
Expand All @@ -103,6 +105,28 @@ suite('fixer.ts', () => {
expect(fix.range.start.isEqual(new vscode.Position(7, Number.MAX_SAFE_INTEGER))).to.equal(true, 'Should be at the end of the violation line');
});

test('Does not add suppression if suppression for that same line already exists', async () => {
existingFixes.add(7);
// Create our fake diagnostic whose start position is the same as the existing fix already added
const diag = new vscode.Diagnostic(
new vscode.Range(
new vscode.Position(7, 0),
new vscode.Position(8, 0)
),
'This message is unimportant',
vscode.DiagnosticSeverity.Warning
);

// Instantiate our fixer.
const fixGenerator: _PmdFixGenerator = new _PmdFixGenerator(doc, diag);

// Attempt to generate fixes for the file.
const fixes: vscode.CodeAction[] = fixGenerator.generateFixes(existingFixes);

// We expect to get no fix, since there is already a fix
expect(fixes).to.have.lengthOf(0, 'Wrong action count');
});

/*
THIS TEST IS DISABLED FOR NOW. WHEN WE FIX BUG W-13816110, ENABLE THIS TEST.
test('Injects suppression at start of existing end-of-line comment', () => {
Expand Down