From b502796346cc75b913ad72b0cd1b7d534f7cb913 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sat, 23 Nov 2024 19:38:12 -0800 Subject: [PATCH 1/8] Use spawn instead of exec for invoking ameba Refactor Task{Queue,} to use vscode's CancellationToken Add output channel for logging state of extension --- package.json | 7 ++--- src/ameba.ts | 75 ++++++++++++++++++++++++++++++++++++++-------- src/amebaOutput.ts | 2 +- src/extension.ts | 74 ++++++++++++++++++++++++++++++++++++++------- src/taskQueue.ts | 61 +++++++++++++------------------------ yarn.lock | 12 +++++++- 6 files changed, 161 insertions(+), 70 deletions(-) diff --git a/package.json b/package.json index 104616a..d84d1ce 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,7 @@ "version": "0.2.1", "license": "MIT", "engines": { - "vscode": "^1.62.0" + "vscode": "^1.75.0" }, "repository": { "type": "git", @@ -18,8 +18,7 @@ "Linters" ], "activationEvents": [ - "onLanguage:crystal", - "onCommand:crystal.ameba.lint" + "onLanguage:crystal" ], "main": "./out/extension.js", "contributes": { @@ -55,9 +54,9 @@ "devDependencies": { "@types/mocha": "^10.0.9", "@types/node": "^22.9.0", + "@types/vscode": "^1.75.0", "tslint": "^6.1.3", "typescript": "^5.6.3", - "@types/vscode": "^1.95.0", "vscode-test": "^1.6.1" } } diff --git a/src/ameba.ts b/src/ameba.ts index c78b476..07c0ba6 100644 --- a/src/ameba.ts +++ b/src/ameba.ts @@ -1,4 +1,4 @@ -import { exec } from 'child_process'; +import { spawn } from 'child_process'; import * as path from 'path'; import { existsSync } from 'fs'; import { @@ -12,9 +12,12 @@ import { window, workspace } from 'vscode'; + import { AmebaOutput } from './amebaOutput'; import { AmebaConfig, getConfig } from './configuration'; import { Task, TaskQueue } from './taskQueue'; +import { outputChannel } from './extension'; + export class Ameba { private diag: DiagnosticCollection; @@ -38,12 +41,39 @@ export class Ameba { if (existsSync(configFile)) args.push('--config', configFile); const task = new Task(document.uri, token => { - const proc = exec(args.join(' '), (err, stdout, stderr) => { - if (token.isCanceled) return; + let stdoutArr: string[] = []; + let stderrArr: string[] = []; + + outputChannel.appendLine(`$ ${args.join(' ')}`) + const proc = spawn(args[0], args.slice(1), { cwd: dir }); + + token.onCancellationRequested(_ => { + proc.kill(); + }) + + proc.stdout.on('data', (data) => { + stdoutArr.push(data.toString()); + }) + + proc.stderr.on('data', (data) => { + stderrArr.push(data.toString()); + }) + + proc.on('error', (err) => { + console.error('Failed to start subprocess:', err); + window.showErrorMessage(`Failed to start Ameba: ${err.message}`) + }) + + proc.on('close', (code) => { + if (token.isCancellationRequested) return; + this.diag.delete(document.uri); - if (err && stderr.length) { - if ((process.platform == 'win32' && err.code === 1) || err.code === 127) { + const stdout = stdoutArr.join('') + const stderr = stderrArr.join('') + + if (code !== 0 && stderr.length) { + if ((process.platform == 'win32' && code === 1) || code === 127) { window.showErrorMessage( `Could not execute Ameba file${args[0] === 'ameba' ? '.' : ` at ${args[0]}`}`, 'Disable (workspace)' @@ -58,10 +88,12 @@ export class Ameba { } let results: AmebaOutput; + try { results = JSON.parse(stdout); } catch (err) { console.error(`Ameba: failed parsing JSON: ${err}`); + outputChannel.appendLine(`[Task] Error: failed to parse JSON:\n${stdout}`) window.showErrorMessage('Ameba: failed to parse JSON response.'); return; } @@ -76,9 +108,11 @@ export class Ameba { source.issues.forEach(issue => { let start = issue.location; let end = issue.end_location; + if (!end.line || !end.column) { end = start; } + const range = new Range( start.line - 1, start.column - 1, @@ -91,6 +125,7 @@ export class Ameba { `[${issue.rule_name}] ${issue.message}`, this.parseSeverity(issue.severity) ); + diag.code = { value: "Docs", target: Uri.parse(`https://crystaldoc.info/github/crystal-ameba/ameba/v${results.metadata.ameba_version}/Ameba/Rule/${issue.rule_name}.html`) @@ -99,13 +134,22 @@ export class Ameba { parsed.push(diag); }); - diagnostics.push([document.uri, parsed]); + let diagnosticUri: Uri; + if (path.isAbsolute(source.path)) { + diagnosticUri = Uri.parse(source.path) + } else if (document.isUntitled) { + diagnosticUri = document.uri; + } else { + diagnosticUri = Uri.parse(path.join(dir, source.path)); + } + + outputChannel.appendLine(`[Task] (${path.relative(dir, source.path)}) Found ${parsed.length} issues`) + diagnostics.push([diagnosticUri, parsed]); } this.diag.set(diagnostics); + outputChannel.appendLine(`[Task] Done!`) }); - - return () => proc.kill(); }); this.taskQueue.enqueue(task); @@ -124,11 +168,16 @@ export class Ameba { } } - public clear(document: TextDocument): void { - let uri = document.uri; - if (uri.scheme === 'file') { - this.taskQueue.cancel(uri); - this.diag.delete(uri); + public clear(document: TextDocument | null = null): void { + if (document) { + let uri = document.uri; + if (uri.scheme === 'file') { + this.taskQueue.cancel(uri); + this.diag.delete(uri); + } + } else { + this.taskQueue.clear(); + this.diag.clear(); } } } diff --git a/src/amebaOutput.ts b/src/amebaOutput.ts index 384f786..c219ece 100644 --- a/src/amebaOutput.ts +++ b/src/amebaOutput.ts @@ -32,4 +32,4 @@ export interface AmebaOutput { metadata: AmebaMetadata; sources: Array; summary: AmebaSummary; -} \ No newline at end of file +} diff --git a/src/extension.ts b/src/extension.ts index fac26bb..654d3a1 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -1,17 +1,29 @@ -import { commands, ExtensionContext, languages, window, workspace } from 'vscode'; +import { + commands, ExtensionContext, languages, + TextDocument, Uri, window, + workspace, WorkspaceFolder +} from 'vscode'; +import * as path from 'path' + import { Ameba } from './ameba'; import { getConfig } from './configuration'; + +export const outputChannel = window.createOutputChannel("Crystal Ameba", "log") + export function activate(context: ExtensionContext) { - const diag = languages.createDiagnosticCollection('crystal'); - let ameba: Ameba | null = new Ameba(diag); - context.subscriptions.push(diag); + const diag = languages.createDiagnosticCollection('crystal'); + let ameba: Ameba | null = new Ameba(diag); + context.subscriptions.push(diag); context.subscriptions.push( commands.registerCommand('crystal.ameba.lint', () => { if (ameba) { const editor = window.activeTextEditor; - if (editor) ameba.execute(editor.document); + if (editor) { + outputChannel.appendLine('[Lint] Running ameba on current document') + ameba.execute(editor.document); + } } else { window.showWarningMessage( 'Ameba has been disabled for this workspace.', @@ -23,7 +35,7 @@ export function activate(context: ExtensionContext) { const editor = window.activeTextEditor; if (editor) ameba.execute(editor.document); }, - _ => {} + _ => { } ); } }) @@ -33,8 +45,12 @@ export function activate(context: ExtensionContext) { commands.registerCommand('crystal.ameba.restart', () => { if (ameba) { const editor = window.activeTextEditor; - if (editor) ameba.clear(editor.document); + if (editor) { + outputChannel.appendLine(`[Restart] Clearing diagnostics for ${getRelativePath(editor.document)}`) + ameba.clear(editor.document); + } } else { + outputChannel.appendLine('[Restart] Starting ameba') ameba = new Ameba(diag); } }) @@ -43,15 +59,18 @@ export function activate(context: ExtensionContext) { context.subscriptions.push( commands.registerCommand('crystal.ameba.disable', () => { if (!ameba) return; - const editor = window.activeTextEditor; - if (editor) ameba.clear(editor.document); + outputChannel.appendLine('[Disable] Disabling ameba for this session') + ameba.clear(); ameba = null; }) ); workspace.onDidChangeConfiguration(_ => { if (!ameba) return; + outputChannel.appendLine(`[Config] Reloading diagnostics after config change`) ameba.config = getConfig(); + ameba.clear() + executeAmebaOnWorkspace(ameba) }); workspace.textDocuments.forEach(doc => { @@ -63,7 +82,14 @@ export function activate(context: ExtensionContext) { }); workspace.onDidSaveTextDocument(doc => { - if (ameba && ameba.config.onSave) ameba.execute(doc); + if (ameba && doc.languageId === 'crystal' && !doc.isUntitled && doc.uri.scheme === 'file') { + outputChannel.appendLine(`[Save] Running ameba on ${getRelativePath(doc)}`) + ameba.execute(doc); + } else if (ameba && path.basename(doc.fileName) == ".ameba.yml") { + outputChannel.appendLine(`[Config] Reloading diagnostics after config file change`) + ameba.clear(); + executeAmebaOnWorkspace(ameba); + } }); workspace.onDidCloseTextDocument(doc => { @@ -71,4 +97,30 @@ export function activate(context: ExtensionContext) { }); } -export function deactivate() {} +export function deactivate() { } + +function executeAmebaOnWorkspace(ameba: Ameba | null) { + if (!ameba) return; + + workspace.textDocuments.forEach(doc => { + if (doc.languageId === 'crystal' && !doc.isUntitled && + doc.uri.scheme === 'file' && !doc.isClosed) { + outputChannel.appendLine(`[Init] Running ameba on ${getRelativePath(doc)}`); + ameba.execute(doc); + } + }); +} + +function getRelativePath(document: TextDocument): string { + const space: WorkspaceFolder = workspace.getWorkspaceFolder(document.uri) ?? noWorkspaceFolder + (document.uri) + return path.relative(space.uri.fsPath, document.uri.fsPath) +} + +export function noWorkspaceFolder(uri: Uri): WorkspaceFolder { + return { + uri: Uri.parse(path.dirname(uri.fsPath)), + name: path.basename(path.dirname(uri.fsPath)), + index: -1 + } +} diff --git a/src/taskQueue.ts b/src/taskQueue.ts index 7519077..9c12ffd 100644 --- a/src/taskQueue.ts +++ b/src/taskQueue.ts @@ -1,11 +1,7 @@ -import { Uri } from 'vscode'; +import { CancellationToken, CancellationTokenSource, Uri } from 'vscode'; -export interface TaskToken { - readonly isCanceled: boolean; - finished(): void; -} +import { outputChannel } from './extension'; -export type CancelCallback = () => void; /** * Task with async operation. It will be enqueued to and managed by @@ -14,52 +10,29 @@ export type CancelCallback = () => void; export class Task { public readonly uri: Uri; public isEnqueued: boolean = false; - private body: (token: TaskToken) => CancelCallback; - private isCanceled: boolean = false; - private resolver?: () => void; - private onCancel?: CancelCallback; + private body: (token: CancellationToken) => void; + private cancelTokenSource: CancellationTokenSource = new CancellationTokenSource(); + private cancelToken: CancellationToken = this.cancelTokenSource.token; /** * @param body Function of task body, which returns callback called * when cancelation is requested. You should call * token.finished() after async operation is done. */ - constructor(uri: Uri, body: (token: TaskToken) => CancelCallback) { + constructor(uri: Uri, body: (token: CancellationToken) => void) { this.uri = uri; this.body = body; } - public run(): Promise { - if (this.isCanceled) return Promise.resolve(); + public run(): void { + if (this.cancelToken.isCancellationRequested) return; const task = this; - return new Promise(resolve => { - task.resolver = () => resolve(); - const token = { - get isCanceled(): boolean { - return task.isCanceled; - }, - - finished(): void { - task.resolveOnce(); - }, - }; - task.onCancel = this.body(token); - }); + return task.body(this.cancelToken); } public cancel(): void { - if (this.isCanceled) return; - this.isCanceled = true; - if (this.onCancel) this.onCancel(); - this.resolveOnce(); - } - - private resolveOnce(): void { - if (this.resolver) { - this.resolver(); - this.resolver = undefined; - } + this.cancelTokenSource.cancel() } } @@ -83,6 +56,7 @@ export class TaskQueue { task.isEnqueued = true; this.tasks.push(task); this.kick(); + outputChannel.appendLine(`[Task] ${this.tasks.length} tasks in queue`) } public cancel(uri: Uri): void { @@ -94,6 +68,12 @@ export class TaskQueue { }); } + public clear(): void { + this.tasks.forEach(task => { + task.cancel(); + }) + } + private async kick(): Promise { if (this.busy) return; this.busy = true; @@ -106,11 +86,12 @@ export class TaskQueue { } try { - await task.run(); + task.run(); } catch (err) { console.error('Error while running ameba:', err); + } finally { + this.tasks.shift(); } - this.tasks.shift(); } } -} \ No newline at end of file +} diff --git a/yarn.lock b/yarn.lock index 8b528a2..f8a17f1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -40,7 +40,12 @@ dependencies: undici-types "~6.19.8" -"@types/vscode@^1.95.0": +"@types/semver@^7.5.8": + version "7.5.8" + resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.5.8.tgz#8268a8c57a3e4abd25c165ecd36237db7948a55e" + integrity sha512-I8EUhyrgfLrcTkzV3TSsGyl1tSuPrEDzr0yd5m90UgNxQkyDXULk3b6MlQqTCpZpNtWe1K0hzclnZkTcLBe2UQ== + +"@types/vscode@^1.75.0": version "1.95.0" resolved "https://registry.yarnpkg.com/@types/vscode/-/vscode-1.95.0.tgz#484aee82c69fa2d73e29d4bf2a91191e570dbc70" integrity sha512-0LBD8TEiNbet3NvWsmn59zLzOFu/txSlGxnv5yAFHCrhG9WvAnR3IvfHzMOs2aeWqgvNjq9pO99IUw8d3n+unw== @@ -385,6 +390,11 @@ semver@^5.3.0: resolved "https://registry.yarnpkg.com/semver/-/semver-5.7.2.tgz#48d55db737c3287cd4835e17fa13feace1c41ef8" integrity sha512-cBznnQ9KjJqU67B52RMC65CMarK2600WFnbkcaiwWq3xy/5haFJlshgnpjovMVJ+Hff49d8GEn0b87C5pDQ10g== +semver@^7.6.3: + version "7.6.3" + resolved "https://registry.yarnpkg.com/semver/-/semver-7.6.3.tgz#980f7b5550bc175fb4dc09403085627f9eb33143" + integrity sha512-oVekP1cKtI+CTDvHWYFUcMtsK/00wmAEfyqKfNdARm8u1wNVhSgaX7A8d4UuIlUI5e84iEwOhs7ZPYRmzU9U6A== + setimmediate@~1.0.4: version "1.0.5" resolved "https://registry.yarnpkg.com/setimmediate/-/setimmediate-1.0.5.tgz#290cbb232e306942d7d7ea9b83732ab7856f8285" From 415eed567c3c71f83314866aacbd6e0490cfc6ad Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sat, 23 Nov 2024 19:51:04 -0800 Subject: [PATCH 2/8] Move queue size log message --- src/taskQueue.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/taskQueue.ts b/src/taskQueue.ts index 9c12ffd..c6db4c3 100644 --- a/src/taskQueue.ts +++ b/src/taskQueue.ts @@ -56,7 +56,6 @@ export class TaskQueue { task.isEnqueued = true; this.tasks.push(task); this.kick(); - outputChannel.appendLine(`[Task] ${this.tasks.length} tasks in queue`) } public cancel(uri: Uri): void { @@ -80,6 +79,8 @@ export class TaskQueue { while (true) { let task: Task | undefined = this.tasks[0]; + outputChannel.appendLine(`[Task] ${this.tasks.length} tasks in queue`); + if (!task) { this.busy = false; return; From 1749250448027c525967f82725494e7b542c96ba Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sat, 23 Nov 2024 19:57:28 -0800 Subject: [PATCH 3/8] Small fix --- src/extension.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/extension.ts b/src/extension.ts index 654d3a1..4d1893e 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -73,9 +73,7 @@ export function activate(context: ExtensionContext) { executeAmebaOnWorkspace(ameba) }); - workspace.textDocuments.forEach(doc => { - ameba && ameba.execute(doc); - }); + executeAmebaOnWorkspace(ameba); workspace.onDidOpenTextDocument(doc => { ameba && ameba.execute(doc); From 3b89d6f669d8112afc150e725d05d143c413ae86 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sun, 24 Nov 2024 03:03:41 -0500 Subject: [PATCH 4/8] Fix task queue not cancelling files, and not waiting for current task to finish before executing next task --- src/ameba.ts | 210 +++++++++++++++++++++++++---------------------- src/taskQueue.ts | 15 ++-- 2 files changed, 120 insertions(+), 105 deletions(-) diff --git a/src/ameba.ts b/src/ameba.ts index 07c0ba6..3cb5df6 100644 --- a/src/ameba.ts +++ b/src/ameba.ts @@ -41,115 +41,129 @@ export class Ameba { if (existsSync(configFile)) args.push('--config', configFile); const task = new Task(document.uri, token => { - let stdoutArr: string[] = []; - let stderrArr: string[] = []; - - outputChannel.appendLine(`$ ${args.join(' ')}`) - const proc = spawn(args[0], args.slice(1), { cwd: dir }); - - token.onCancellationRequested(_ => { - proc.kill(); - }) - - proc.stdout.on('data', (data) => { - stdoutArr.push(data.toString()); - }) - - proc.stderr.on('data', (data) => { - stderrArr.push(data.toString()); - }) - - proc.on('error', (err) => { - console.error('Failed to start subprocess:', err); - window.showErrorMessage(`Failed to start Ameba: ${err.message}`) - }) - - proc.on('close', (code) => { - if (token.isCancellationRequested) return; - - this.diag.delete(document.uri); - - const stdout = stdoutArr.join('') - const stderr = stderrArr.join('') - - if (code !== 0 && stderr.length) { - if ((process.platform == 'win32' && code === 1) || code === 127) { - window.showErrorMessage( - `Could not execute Ameba file${args[0] === 'ameba' ? '.' : ` at ${args[0]}`}`, - 'Disable (workspace)' - ).then( - disable => disable && commands.executeCommand('crystal.ameba.disable'), - _ => { } - ); - } else { - window.showErrorMessage(stderr); + return new Promise((resolve, reject) => { + let stdoutArr: string[] = []; + let stderrArr: string[] = []; + + outputChannel.appendLine(`$ ${args.join(' ')}`) + const proc = spawn(args[0], args.slice(1), { cwd: dir }); + + token.onCancellationRequested(_ => { + proc.kill(); + }) + + proc.stdout.on('data', (data) => { + stdoutArr.push(data.toString()); + }) + + proc.stderr.on('data', (data) => { + stderrArr.push(data.toString()); + }) + + proc.on('error', (err) => { + console.error('Failed to start subprocess:', err); + window.showErrorMessage(`Failed to start Ameba: ${err.message}`) + reject(err); + }) + + proc.on('close', (code) => { + if (token.isCancellationRequested) { + resolve(); + return; } - return; - } - let results: AmebaOutput; - - try { - results = JSON.parse(stdout); - } catch (err) { - console.error(`Ameba: failed parsing JSON: ${err}`); - outputChannel.appendLine(`[Task] Error: failed to parse JSON:\n${stdout}`) - window.showErrorMessage('Ameba: failed to parse JSON response.'); - return; - } + this.diag.delete(document.uri); + + const stdout = stdoutArr.join('') + const stderr = stderrArr.join('') + + if (code !== 0 && stderr.length) { + if ((process.platform == 'win32' && code === 1) || code === 127) { + window.showErrorMessage( + `Could not execute Ameba file${args[0] === 'ameba' ? '.' : ` at ${args[0]}`}`, + 'Disable (workspace)' + ).then( + disable => disable && commands.executeCommand('crystal.ameba.disable'), + _ => { } + ); + } else { + window.showErrorMessage(stderr); + } - if (!results.summary.issues_count) return; - const diagnostics: [Uri, Diagnostic[]][] = []; + reject(new Error(stderr)); + return; + } - for (let source of results.sources) { - if (!source.issues.length) continue; - let parsed: Diagnostic[] = []; + let results: AmebaOutput; - source.issues.forEach(issue => { - let start = issue.location; - let end = issue.end_location; + try { + results = JSON.parse(stdout); + } catch (err) { + console.error(`Ameba: failed parsing JSON: ${err}`); + outputChannel.appendLine(`[Task] Error: failed to parse JSON:\n${stdout}`) + window.showErrorMessage('Ameba: failed to parse JSON response.'); + reject(err); + return; + } - if (!end.line || !end.column) { - end = start; - } + if (!results.summary.issues_count) { + resolve(); + return; + } - const range = new Range( - start.line - 1, - start.column - 1, - end.line - 1, - end.column - ); - - const diag = new Diagnostic( - range, - `[${issue.rule_name}] ${issue.message}`, - this.parseSeverity(issue.severity) - ); - - diag.code = { - value: "Docs", - target: Uri.parse(`https://crystaldoc.info/github/crystal-ameba/ameba/v${results.metadata.ameba_version}/Ameba/Rule/${issue.rule_name}.html`) + const diagnostics: [Uri, Diagnostic[]][] = []; + + for (let source of results.sources) { + if (!source.issues.length) continue; + let parsed: Diagnostic[] = []; + + source.issues.forEach(issue => { + let start = issue.location; + let end = issue.end_location; + + if (!end.line || !end.column) { + end = start; + } + + const range = new Range( + start.line - 1, + start.column - 1, + end.line - 1, + end.column + ); + + const diag = new Diagnostic( + range, + `[${issue.rule_name}] ${issue.message}`, + this.parseSeverity(issue.severity) + ); + + diag.code = { + value: "Docs", + target: Uri.parse(`https://crystaldoc.info/github/crystal-ameba/ameba/v${results.metadata.ameba_version}/Ameba/Rule/${issue.rule_name}.html`) + } + + parsed.push(diag); + }); + + let diagnosticUri: Uri; + if (path.isAbsolute(source.path)) { + diagnosticUri = Uri.parse(source.path) + } else if (document.isUntitled) { + diagnosticUri = document.uri; + } else { + diagnosticUri = Uri.parse(path.join(dir, source.path)); } - parsed.push(diag); - }); - - let diagnosticUri: Uri; - if (path.isAbsolute(source.path)) { - diagnosticUri = Uri.parse(source.path) - } else if (document.isUntitled) { - diagnosticUri = document.uri; - } else { - diagnosticUri = Uri.parse(path.join(dir, source.path)); + outputChannel.appendLine(`[Task] (${path.relative(dir, source.path)}) Found ${parsed.length} issues`) + diagnostics.push([diagnosticUri, parsed]); } - outputChannel.appendLine(`[Task] (${path.relative(dir, source.path)}) Found ${parsed.length} issues`) - diagnostics.push([diagnosticUri, parsed]); - } - - this.diag.set(diagnostics); - outputChannel.appendLine(`[Task] Done!`) - }); + this.diag.set(diagnostics); + outputChannel.appendLine(`[Task] Done!`) + resolve(); + }); + }) }); this.taskQueue.enqueue(task); diff --git a/src/taskQueue.ts b/src/taskQueue.ts index c6db4c3..fcddc97 100644 --- a/src/taskQueue.ts +++ b/src/taskQueue.ts @@ -10,7 +10,7 @@ import { outputChannel } from './extension'; export class Task { public readonly uri: Uri; public isEnqueued: boolean = false; - private body: (token: CancellationToken) => void; + private body: (token: CancellationToken) => Promise; private cancelTokenSource: CancellationTokenSource = new CancellationTokenSource(); private cancelToken: CancellationToken = this.cancelTokenSource.token; @@ -19,16 +19,16 @@ export class Task { * when cancelation is requested. You should call * token.finished() after async operation is done. */ - constructor(uri: Uri, body: (token: CancellationToken) => void) { + constructor(uri: Uri, body: (token: CancellationToken) => Promise) { this.uri = uri; this.body = body; } - public run(): void { + public async run(): Promise { if (this.cancelToken.isCancellationRequested) return; const task = this; - return task.body(this.cancelToken); + return await task.body(this.cancelToken); } public cancel(): void { @@ -60,11 +60,12 @@ export class TaskQueue { public cancel(uri: Uri): void { const uriString = uri.toString(true); - this.tasks.forEach(task => { + + for (const task of this.tasks) { if (task.uri.toString(true) === uriString) { task.cancel(); } - }); + } } public clear(): void { @@ -87,7 +88,7 @@ export class TaskQueue { } try { - task.run(); + await task.run(); } catch (err) { console.error('Error while running ameba:', err); } finally { From 1ea892db4a68a98f8349d694b4d1ee9966be3c67 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sun, 24 Nov 2024 03:24:17 -0500 Subject: [PATCH 5/8] Fix relative filename in log --- src/ameba.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ameba.ts b/src/ameba.ts index 3cb5df6..6ffd488 100644 --- a/src/ameba.ts +++ b/src/ameba.ts @@ -155,7 +155,8 @@ export class Ameba { diagnosticUri = Uri.parse(path.join(dir, source.path)); } - outputChannel.appendLine(`[Task] (${path.relative(dir, source.path)}) Found ${parsed.length} issues`) + const logPath = path.relative(dir, diagnosticUri.fsPath) + outputChannel.appendLine(`[Task] (${logPath}) Found ${parsed.length} issues`) diagnostics.push([diagnosticUri, parsed]); } From 51fc85c4946ced881f93fecee2b31c2bc2832170 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sat, 23 Nov 2024 20:32:49 -0800 Subject: [PATCH 6/8] Functionality to lint while typing Added configuration option `lint-trigger` which defaults to save or type depending on the current version of ameba in use. This will also lint untitled files that aren't saved to disk. --- package.json | 22 +++++++++++++++++++++- src/ameba.ts | 28 ++++++++++++++++++++++------ src/configuration.ts | 31 ++++++++++++++++++++++++++++--- src/extension.ts | 30 +++++++++++++++++++++++++++--- 4 files changed, 98 insertions(+), 13 deletions(-) diff --git a/package.json b/package.json index d84d1ce..5c278cc 100644 --- a/package.json +++ b/package.json @@ -43,7 +43,23 @@ "command": "crystal.ameba.disable", "title": "Crystal Ameba: disable lints (workspace)" } - ] + ], + "configuration": { + "type": "object", + "title": "Crystal Ameba configuration", + "properties": { + "crystal-ameba.lint-trigger": { + "type": "string", + "description": "When the linter should be executed. Set to `none` to disable automatic linting.", + "default": "type", + "enum": [ + "none", + "save", + "type" + ] + } + } + } }, "scripts": { "vscode:prepublish": "yarn run compile", @@ -55,8 +71,12 @@ "@types/mocha": "^10.0.9", "@types/node": "^22.9.0", "@types/vscode": "^1.75.0", + "@types/semver": "^7.5.8", "tslint": "^6.1.3", "typescript": "^5.6.3", "vscode-test": "^1.6.1" + }, + "dependencies": { + "semver": "^7.6.3" } } diff --git a/src/ameba.ts b/src/ameba.ts index 6ffd488..fb31f94 100644 --- a/src/ameba.ts +++ b/src/ameba.ts @@ -16,7 +16,7 @@ import { import { AmebaOutput } from './amebaOutput'; import { AmebaConfig, getConfig } from './configuration'; import { Task, TaskQueue } from './taskQueue'; -import { outputChannel } from './extension'; +import { documentIsVirtual, noWorkspaceFolder, outputChannel } from './extension'; export class Ameba { @@ -30,13 +30,23 @@ export class Ameba { this.config = getConfig(); } - public execute(document: TextDocument): void { - if (document.languageId !== 'crystal' || document.isUntitled || document.uri.scheme !== 'file') { - return; + public execute(document: TextDocument, virtual: boolean = false): void { + if (document.languageId !== 'crystal') return; + if (documentIsVirtual(document) && !virtual) return; + + const dir = (workspace.getWorkspaceFolder(document.uri) ?? noWorkspaceFolder(document.uri)).uri.fsPath; + + const args = [this.config.command, '--format', 'json']; + + if (!virtual) { + args.push(document.fileName) + } else { + args.push('--stdin-filename', document.fileName); + + // Disabling these as they're common when typing + args.push('--except', 'Lint/Formatting,Layout/TrailingBlankLines,Layout/TrailingWhitespace,Naming/Filename'); } - const args = [this.config.command, document.fileName, '--format', 'json']; - const dir = workspace.getWorkspaceFolder(document.uri)!.uri.fsPath; const configFile = path.join(dir, this.config.configFileName); if (existsSync(configFile)) args.push('--config', configFile); @@ -48,6 +58,12 @@ export class Ameba { outputChannel.appendLine(`$ ${args.join(' ')}`) const proc = spawn(args[0], args.slice(1), { cwd: dir }); + if (virtual) { + const documentText: string = document.getText(); + proc.stdin.write(documentText) + proc.stdin.end(); + } + token.onCancellationRequested(_ => { proc.kill(); }) diff --git a/src/configuration.ts b/src/configuration.ts index f09f25e..22b0596 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -1,11 +1,22 @@ import { workspace } from 'vscode'; import * as path from 'path'; import { existsSync } from 'fs'; +import * as semver from 'semver'; +import { execSync } from 'child_process'; + +import { outputChannel } from './extension'; + export interface AmebaConfig { command: string; configFileName: string; - onSave: boolean; + trigger: LintTrigger; +} + +export enum LintTrigger { + None = "none", + Save = "save", + Type = "type" } export function getConfig(): AmebaConfig { @@ -13,12 +24,26 @@ export function getConfig(): AmebaConfig { const root = workspace.workspaceFolders || []; if (root.length) { const localAmebaPath = path.join(root[0].uri.fsPath, 'bin', 'ameba'); - if (existsSync(localAmebaPath)) command = localAmebaPath; + if (existsSync(localAmebaPath)) { + outputChannel.appendLine(`[Config] Using local ameba at ${localAmebaPath}`) + command = localAmebaPath; + } else { + outputChannel.appendLine(`[Config] Using system ameba`) + } + } + + const workspaceConfig = workspace.getConfiguration('crystal-ameba'); + const currentVersion = execSync(`"${command}" --version`).toString(); + + let trigger = workspaceConfig.get("lint-trigger", LintTrigger.Type); + + if (!semver.satisfies(currentVersion, ">=1.6.4") && trigger == LintTrigger.Type) { + trigger = LintTrigger.Save; } return { command, configFileName: '.ameba.yml', - onSave: true + trigger: trigger }; }; diff --git a/src/extension.ts b/src/extension.ts index 4d1893e..35ebf25 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -6,7 +6,7 @@ import { import * as path from 'path' import { Ameba } from './ameba'; -import { getConfig } from './configuration'; +import { getConfig, LintTrigger } from './configuration'; export const outputChannel = window.createOutputChannel("Crystal Ameba", "log") @@ -14,6 +14,7 @@ export const outputChannel = window.createOutputChannel("Crystal Ameba", "log") export function activate(context: ExtensionContext) { const diag = languages.createDiagnosticCollection('crystal'); let ameba: Ameba | null = new Ameba(diag); + context.subscriptions.push(diag); context.subscriptions.push( @@ -75,12 +76,31 @@ export function activate(context: ExtensionContext) { executeAmebaOnWorkspace(ameba); + // This can happen when a file is open _or_ when a file's language id changes workspace.onDidOpenTextDocument(doc => { - ameba && ameba.execute(doc); + if (ameba && doc.languageId === 'crystal') { + if (documentIsVirtual(doc)) { + if (ameba.config.trigger === LintTrigger.Type) { + outputChannel.appendLine(`[Open] Running ameba on ${getRelativePath(doc)}`); + ameba.execute(doc, true); + } + } else { + outputChannel.appendLine(`[Open] Running ameba on ${getRelativePath(doc)}`); + ameba.execute(doc); + } + } }); + workspace.onDidChangeTextDocument(e => { + if (ameba && ameba.config.trigger == LintTrigger.Type && e.document.languageId === 'crystal') { + outputChannel.appendLine(`[Change] Running ameba on ${getRelativePath(e.document)}`); + ameba.execute(e.document, true); + } + }) + workspace.onDidSaveTextDocument(doc => { - if (ameba && doc.languageId === 'crystal' && !doc.isUntitled && doc.uri.scheme === 'file') { + if (ameba && ameba.config.trigger === LintTrigger.Save && + doc.languageId === 'crystal' && !documentIsVirtual(doc)) { outputChannel.appendLine(`[Save] Running ameba on ${getRelativePath(doc)}`) ameba.execute(doc); } else if (ameba && path.basename(doc.fileName) == ".ameba.yml") { @@ -122,3 +142,7 @@ export function noWorkspaceFolder(uri: Uri): WorkspaceFolder { index: -1 } } + +export function documentIsVirtual(document: TextDocument): boolean { + return document.isDirty || document.isUntitled || document.uri.scheme !== 'file' +} From 6a8b825d14e3515b907a73e3823b7eb3b4bc6504 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sun, 24 Nov 2024 03:50:41 -0500 Subject: [PATCH 7/8] Don't run linter when lint type is set to none --- src/extension.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extension.ts b/src/extension.ts index 35ebf25..b509c7a 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -78,7 +78,7 @@ export function activate(context: ExtensionContext) { // This can happen when a file is open _or_ when a file's language id changes workspace.onDidOpenTextDocument(doc => { - if (ameba && doc.languageId === 'crystal') { + if (ameba && doc.languageId === 'crystal' && ameba.config.trigger !== LintTrigger.None) { if (documentIsVirtual(doc)) { if (ameba.config.trigger === LintTrigger.Type) { outputChannel.appendLine(`[Open] Running ameba on ${getRelativePath(doc)}`); From 3175c433d0e5325c5bed89e00c4283c07a425261 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sun, 24 Nov 2024 03:01:02 -0500 Subject: [PATCH 8/8] Add `lint-scope` config option, which tells the linter to either run only on open files or the entire workspace Modify the ameba execute function to take either a workspace folder or a text document --- package.json | 13 ++++++++++ src/ameba.ts | 32 ++++++++++++++--------- src/configuration.ts | 10 +++++++- src/extension.ts | 61 +++++++++++++++++++++++++++++++++++++------- 4 files changed, 94 insertions(+), 22 deletions(-) diff --git a/package.json b/package.json index 5c278cc..2e4e9f3 100644 --- a/package.json +++ b/package.json @@ -42,12 +42,25 @@ { "command": "crystal.ameba.disable", "title": "Crystal Ameba: disable lints (workspace)" + }, + { + "command": "crystal.ameba.lint-workspace", + "title": "Crystal Ameba: lint all files in workspace" } ], "configuration": { "type": "object", "title": "Crystal Ameba configuration", "properties": { + "crystal-ameba.lint-scope": { + "type": "string", + "description": "Whether the linter should only care about open files or all files in the workspace.", + "default": "file", + "enum": [ + "file", + "workspace" + ] + }, "crystal-ameba.lint-trigger": { "type": "string", "description": "When the linter should be executed. Set to `none` to disable automatic linting.", diff --git a/src/ameba.ts b/src/ameba.ts index fb31f94..cf1c20f 100644 --- a/src/ameba.ts +++ b/src/ameba.ts @@ -10,7 +10,8 @@ import { TextDocument, Uri, window, - workspace + workspace, + WorkspaceFolder } from 'vscode'; import { AmebaOutput } from './amebaOutput'; @@ -30,21 +31,28 @@ export class Ameba { this.config = getConfig(); } - public execute(document: TextDocument, virtual: boolean = false): void { - if (document.languageId !== 'crystal') return; - if (documentIsVirtual(document) && !virtual) return; + public execute(document: TextDocument | WorkspaceFolder, virtual: boolean = false): void { + const isWorkspace = !('languageId' in document); + if (isWorkspace) virtual = false; + + if (!isWorkspace) { + if (document.languageId !== 'crystal') return; + if (documentIsVirtual(document) && !virtual) return; + } const dir = (workspace.getWorkspaceFolder(document.uri) ?? noWorkspaceFolder(document.uri)).uri.fsPath; const args = [this.config.command, '--format', 'json']; - if (!virtual) { - args.push(document.fileName) - } else { - args.push('--stdin-filename', document.fileName); + if (!isWorkspace) { + if (!virtual) { + args.push(document.fileName) + } else { + args.push('--stdin-filename', document.fileName); - // Disabling these as they're common when typing - args.push('--except', 'Lint/Formatting,Layout/TrailingBlankLines,Layout/TrailingWhitespace,Naming/Filename'); + // Disabling these as they're common when typing + args.push('--except', 'Lint/Formatting,Layout/TrailingBlankLines,Layout/TrailingWhitespace,Naming/Filename'); + } } const configFile = path.join(dir, this.config.configFileName); @@ -58,7 +66,7 @@ export class Ameba { outputChannel.appendLine(`$ ${args.join(' ')}`) const proc = spawn(args[0], args.slice(1), { cwd: dir }); - if (virtual) { + if (virtual && !isWorkspace) { const documentText: string = document.getText(); proc.stdin.write(documentText) proc.stdin.end(); @@ -165,7 +173,7 @@ export class Ameba { let diagnosticUri: Uri; if (path.isAbsolute(source.path)) { diagnosticUri = Uri.parse(source.path) - } else if (document.isUntitled) { + } else if (!isWorkspace && document.isUntitled) { diagnosticUri = document.uri; } else { diagnosticUri = Uri.parse(path.join(dir, source.path)); diff --git a/src/configuration.ts b/src/configuration.ts index 22b0596..bca25c4 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -11,6 +11,7 @@ export interface AmebaConfig { command: string; configFileName: string; trigger: LintTrigger; + scope: LintScope; } export enum LintTrigger { @@ -19,6 +20,11 @@ export enum LintTrigger { Type = "type" } +export enum LintScope { + File = "file", + Workspace = "workspace" +} + export function getConfig(): AmebaConfig { let command = 'ameba'; const root = workspace.workspaceFolders || []; @@ -35,6 +41,7 @@ export function getConfig(): AmebaConfig { const workspaceConfig = workspace.getConfiguration('crystal-ameba'); const currentVersion = execSync(`"${command}" --version`).toString(); + const scope = workspaceConfig.get("lint-scope", LintScope.File); let trigger = workspaceConfig.get("lint-trigger", LintTrigger.Type); if (!semver.satisfies(currentVersion, ">=1.6.4") && trigger == LintTrigger.Type) { @@ -44,6 +51,7 @@ export function getConfig(): AmebaConfig { return { command, configFileName: '.ameba.yml', - trigger: trigger + trigger: trigger, + scope: scope }; }; diff --git a/src/extension.ts b/src/extension.ts index b509c7a..2a6e79e 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -6,7 +6,7 @@ import { import * as path from 'path' import { Ameba } from './ameba'; -import { getConfig, LintTrigger } from './configuration'; +import { getConfig, LintScope, LintTrigger } from './configuration'; export const outputChannel = window.createOutputChannel("Crystal Ameba", "log") @@ -42,6 +42,28 @@ export function activate(context: ExtensionContext) { }) ); + context.subscriptions.push( + commands.registerCommand('crystal.ameba.lint-workspace', () => { + if (ameba) { + if (!ameba) return; + outputChannel.appendLine('[Lint] Running ameba on current workspace') + executeAmebaOnWorkspace(ameba) + } else { + window.showWarningMessage( + 'Ameba has been disabled for this workspace.', + 'Enable' + ).then( + enable => { + if (!enable) return; + ameba = new Ameba(diag); + executeAmebaOnWorkspace(ameba) + }, + _ => { } + ); + } + }) + ) + context.subscriptions.push( commands.registerCommand('crystal.ameba.restart', () => { if (ameba) { @@ -111,7 +133,21 @@ export function activate(context: ExtensionContext) { }); workspace.onDidCloseTextDocument(doc => { - ameba && ameba.clear(doc); + if (!ameba) return; + let shouldClear = false; + + if (ameba.config.scope == LintScope.File) { + shouldClear = true; + } else if (workspace.workspaceFolders) { + shouldClear = !workspace.getWorkspaceFolder(doc.uri); + } else { + shouldClear = true; + } + + if (shouldClear) { + outputChannel.appendLine(`[Clear] Clearing ${getRelativePath(doc)}`) + ameba.clear(doc); + } }); } @@ -120,13 +156,20 @@ export function deactivate() { } function executeAmebaOnWorkspace(ameba: Ameba | null) { if (!ameba) return; - workspace.textDocuments.forEach(doc => { - if (doc.languageId === 'crystal' && !doc.isUntitled && - doc.uri.scheme === 'file' && !doc.isClosed) { - outputChannel.appendLine(`[Init] Running ameba on ${getRelativePath(doc)}`); - ameba.execute(doc); - } - }); + if (ameba.config.scope === LintScope.File) { + workspace.textDocuments.forEach(doc => { + if (doc.languageId === 'crystal' && !doc.isUntitled && + doc.uri.scheme === 'file' && !doc.isClosed) { + outputChannel.appendLine(`[Init] Running ameba on ${getRelativePath(doc)}`); + ameba.execute(doc); + } + }); + } else { + workspace.workspaceFolders?.forEach(folder => { + outputChannel.appendLine(`[Init] Running ameba on ${folder.name}`); + ameba.execute(folder); + }) + } } function getRelativePath(document: TextDocument): string {