Add shortcut to DiagnosticAdviceAction
Fix git change extraction to work with padded lines and ?? prefixes
Don't attach suppression comments to Program
Turn identifier camelCase autofix to a suggestion as it's unsafe
Make `--review` rerun the command after every action. Have some smartness to dedupe ignored diagnostics.
MODIFY
packages/@romejs/core/client/review.ts
packages/@romejs/core/client/review.ts
import ClientRequest from './ClientRequest';
import {printDiagnostics} from '@romejs/cli-diagnostics';
import {SelectOption} from '@romejs/cli-reporter';
-import {DiagnosticAdviceAction} from '@romejs/diagnostics';
+import {
+ Diagnostic,
+ DiagnosticAdviceAction,
+ derivePositionlessKeyFromDiagnostic,
+} from '@romejs/diagnostics';
import {MasterQueryResponse} from '../common/bridges/MasterBridge';
import {ClientRequestFlags} from '../common/types/client';
import {localCommands} from './commands';
import {masterCommands} from '../master/commands';
+import {Dict} from '@romejs/typescript-helpers';
+import {EMPTY_SUCCESS_RESPONSE} from '../master/MasterRequest';
-export default async function review(
+type State = {
+ seen: Set<string>;
+ resolvedCount: number;
+};
+
+async function check(
req: ClientRequest,
+ state: State,
): Promise<MasterQueryResponse> {
const {client} = req;
+ const {reporter} = client;
const res = await req.fork({
...req.query,
@@ -25,11 +38,6 @@ export default async function review(
noData: false,
}).initCommand();
- const {requestFlags} = req.query;
- if (requestFlags === undefined || !requestFlags.review) {
- return res;
- }
-
if (res.type === 'SUCCESS') {
throw new Error('Expected diagnostics or an error');
}
@@ -39,22 +47,24 @@ export default async function review(
return res;
}
- const {reporter} = client;
+ const diagnostics = res.diagnostics;
+ let diag: undefined | Diagnostic;
- for (let diag of res.diagnostics) {
- const {filename, start} = diag.location;
- if (filename === undefined || start === undefined) {
- continue;
+ for (const _diag of diagnostics) {
+ const key = derivePositionlessKeyFromDiagnostic(_diag);
+ if (!state.seen.has(key)) {
+ state.seen.add(key);
+ diag = _diag;
+ break;
}
-
- let {category, advice = []} = diag.description;
- if (category === 'lint/pendingFixes') {
- continue;
}
- reporter.clearScreen();
+ if (diag === undefined) {
+ return res;
+ }
// Extract actions and remove them from the diagnostic
+ let {advice = []} = diag.description;
const actions: Array<DiagnosticAdviceAction> = [];
for (const item of advice) {
if (item.type === 'action') {
@@ -71,36 +81,39 @@ export default async function review(
};
const optionToAction: Map<string, DiagnosticAdviceAction> = new Map();
+ const chosenShortcuts: Set<string> = new Set(['n', 'escape']);
- const optionsWithIgnoreAndActions: {
- ignore: SelectOption;
- [key: string]: SelectOption;
- } = {
- ignore: {
- label: 'Do nothing',
- shortcut: 'n',
- },
- };
+ const actionOptions: Dict<SelectOption> = {};
let counter = 0;
for (const action of actions) {
const key = String(counter++);
+ let shortcut = action.shortcut !== undefined && !chosenShortcuts.has(
+ action.shortcut,
+ ) ? action.shortcut : undefined;
optionToAction.set(key, action);
- optionsWithIgnoreAndActions[key] = {
+ actionOptions[key] = {
label: action.noun,
+ shortcut,
};
}
const options: {
+ ignore: SelectOption;
exit: SelectOption;
} = {
- ...optionsWithIgnoreAndActions,
+ ignore: {
+ label: 'Do nothing',
+ shortcut: 'n',
+ },
+ ...actionOptions,
exit: {
label: 'Exit',
shortcut: 'escape',
},
};
+ reporter.clearScreen();
printDiagnostics({
diagnostics: [diag],
suppressions: [],
@@ -114,13 +127,17 @@ export default async function review(
options,
});
+ if (answer === 'ignore') {
+ return await check(req, state);
+ }
+
if (answer === 'exit') {
- break;
+ return EMPTY_SUCCESS_RESPONSE;
}
const action = optionToAction.get(answer);
if (action === undefined) {
- throw new Error();
+ throw new Error('Should have found an action for this option');
}
const requestFlags: Partial<ClientRequestFlags> = {
@@ -138,21 +155,54 @@ export default async function review(
requestFlags.allowDirty = true;
}
- const res = await client.query({
+ // Execute action
+ const actionRes = await client.query({
command: action.command,
args: action.args,
commandFlags: action.commandFlags,
requestFlags,
}, 'master');
- if (res.type === 'ERROR' || res.type === 'INVALID_REQUEST') {
- return res;
+ if (actionRes.type === 'ERROR' || actionRes.type === 'INVALID_REQUEST') {
+ return actionRes;
}
+
+ state.resolvedCount++;
+ return await check(req, state);
}
- return {
- type: 'SUCCESS',
- data: undefined,
- hasData: false,
- markers: [],
+export default async function review(
+ req: ClientRequest,
+): Promise<MasterQueryResponse> {
+ const {reporter} = req.client;
+ const state: State = {
+ seen: new Set(),
+ resolvedCount: 0,
};
+ const res = await check(req, state);
+
+ if (state.seen.size === 0) {
+ reporter.success('Nothing to review!');
+ } else {
+ reporter.clearScreen();
+ if (res.type === 'DIAGNOSTICS') {
+ printDiagnostics({
+ diagnostics: res.diagnostics,
+ suppressions: [],
+ excludeFooter: true,
+ printerOptions: {
+ reporter,
+ },
+ });
+ reporter.hr();
+ reporter.error(
+ `<number emphasis>${res.diagnostics.length}</number> unresolved <grammarNumber plural="issues" singular="issue">${res.diagnostics.length}</grammarNumber> remaining`,
+ );
+ }
+
+ reporter.success(
+ `<number emphasis>${state.resolvedCount}</number> <grammarNumber plural="issues" singular="issue">${state.resolvedCount}</grammarNumber> resolved`,
+ );
+ }
+
+ return res;
}

MODIFY
packages/@romejs/core/master/Master.ts
packages/@romejs/core/master/Master.ts
@@ -28,7 +28,10 @@ import {
} from '@romejs/cli-diagnostics';
import {ConsumePath, consume} from '@romejs/consume';
import {Event, EventSubscription} from '@romejs/events';
-import MasterRequest, {MasterRequestInvalid} from './MasterRequest';
+import MasterRequest, {
+ EMPTY_SUCCESS_RESPONSE,
+ MasterRequestInvalid,
+} from './MasterRequest';
import ProjectManager from './project/ProjectManager';
import WorkerManager from './WorkerManager';
import Resolver from './fs/Resolver';
@@ -852,10 +855,9 @@ export default class Master {
// Only the command promise should have won the race with a resolve
const data = await commandPromise;
return {
- type: 'SUCCESS',
+ ...EMPTY_SUCCESS_RESPONSE,
hasData: data !== undefined,
data,
- markers: [],
};
} else {
throw new Error(`Unknown command ${String(query.commandName)}`);
@@ -875,16 +877,7 @@ export default class Master {
message: err.message,
stack: err.stack,
};
- } else if (diagnostics.length === 0) {
- // Maybe DIAGNOSTICS and an empty array still makes sense instead of SUCCESS?
- return {
- type: 'SUCCESS',
- hasData: false,
- data: undefined,
- markers: [],
- };
- } else {
- if (err instanceof MasterRequestInvalid) {
+ } else if (err instanceof MasterRequestInvalid) {
return {
type: 'INVALID_REQUEST',
diagnostics,
@@ -898,7 +891,6 @@ export default class Master {
}
}
}
- }
handleRequestError(req: MasterRequest, rawErr: Error): undefined | Diagnostics {
let err = rawErr;

MODIFY
packages/@romejs/core/master/MasterRequest.ts
packages/@romejs/core/master/MasterRequest.ts
@@ -35,6 +35,7 @@ import {BundlerConfig} from '../common/types/bundler';
import MasterBridge, {
MasterQueryRequest,
MasterQueryResponse,
+ MasterQueryResponseSuccess,
} from '../common/bridges/MasterBridge';
import Master, {
MasterClient,
@@ -104,6 +105,13 @@ type ResolvedArg = {
type ResolvedArgs = Array<ResolvedArg>;
+export const EMPTY_SUCCESS_RESPONSE: MasterQueryResponseSuccess = {
+ type: 'SUCCESS',
+ hasData: false,
+ data: undefined,
+ markers: [],
+};
+
export type MasterRequestGetFilesOptions = Omit<
MemoryFSGlobOptions,
'getProjectIgnore'
@@ -201,10 +209,8 @@ export default class MasterRequest {
if (this.query.noData) {
if (res.type === 'SUCCESS') {
res = {
- type: 'SUCCESS',
+ ...EMPTY_SUCCESS_RESPONSE,
hasData: res.data !== undefined,
- data: undefined,
- markers: [],
};
} else if (res.type === 'DIAGNOSTICS') {
res = {

MODIFY
packages/@romejs/core/master/commands/lint.ts
packages/@romejs/core/master/commands/lint.ts
@@ -41,11 +41,6 @@ export default createMasterCommand<Flags>({
async callback(req: MasterRequest, flags: Flags): Promise<void> {
const {reporter} = req;
- // This wont trigger the shouldSave check at the bottom
- if (req.query.requestFlags.review) {
- await req.assertCleanVSC();
- }
-
let compilerOptionsPerFile: LinterOptions['compilerOptionsPerFile'] = {};
const {decisions} = flags;
if (decisions !== undefined) {

MODIFY
packages/@romejs/core/master/linter/Linter.ts
packages/@romejs/core/master/linter/Linter.ts
@@ -87,19 +87,19 @@ function createDiagnosticsPrinter(
if (isError) {
let hasPendingFixes = false;
- for (const {description} of processor.getDiagnostics()) {
- if (description.category === 'lint/pendingFixes') {
+ for (const {fixable} of processor.getDiagnostics()) {
+ if (fixable) {
hasPendingFixes = true;
}
}
if (hasPendingFixes) {
reporter.info(
- 'Fixes available. Run <command>rome lint --review</command> to review.',
- );
- reporter.info(
- 'Alternatively run <command>rome lint --save</command> to apply recommend fixes and formatting.',
+ 'Fixes available. To apply recommended fixes and formatting run',
);
+ reporter.command('rome lint --save');
+ reporter.info('To choose fix suggestions run');
+ reporter.command('rome lint --review');
}
} else {
if (totalCount === 0) {
@@ -378,7 +378,8 @@ export default class Linter {
shouldSave(): boolean {
const {save, hasDecisions, formatOnly} = this.options;
- return save || hasDecisions || formatOnly;
+ return save || hasDecisions || formatOnly ||
+ this.request.query.requestFlags.review;
}
getFileArgOptions(): MasterRequestGetFilesOptions {

MODIFY
packages/@romejs/core/master/lsp/LSPServer.ts
packages/@romejs/core/master/lsp/LSPServer.ts
@@ -30,7 +30,7 @@ import {
PartialMasterQueryRequest,
} from '@romejs/core/common/bridges/MasterBridge';
import Linter from '../linter/Linter';
-import MasterRequest from '../MasterRequest';
+import MasterRequest, {EMPTY_SUCCESS_RESPONSE} from '../MasterRequest';
import {DEFAULT_CLIENT_REQUEST_FLAGS} from '@romejs/core/common/types/client';
import stringDiff, {Diffs, diffConstants} from '@romejs/string-diff';
import {JSONObject, JSONPropertyValue} from '@romejs/codec-json';
@@ -337,12 +337,7 @@ export default class LSPServer {
// TODO maybe unset all buffers?
const req = this.lintSessions.get(path);
if (req !== undefined) {
- req.teardown({
- type: 'SUCCESS',
- hasData: false,
- data: undefined,
- markers: [],
- });
+ req.teardown(EMPTY_SUCCESS_RESPONSE);
this.lintSessions.delete(path);
}
}

MODIFY
packages/@romejs/core/worker/WorkerAPI.ts
packages/@romejs/core/worker/WorkerAPI.ts
@@ -319,6 +319,7 @@ export default class WorkerAPI {
diagnostics: [
...diagnostics,
{
+ fixable: true,
location: {
filename: ref.uid,
},

MODIFY
packages/@romejs/diagnostics/derive.ts
packages/@romejs/diagnostics/derive.ts
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
- */
-
-import {
+ */import {
Diagnostic,
DiagnosticAdvice,
+ DiagnosticLocation,
DiagnosticOrigin,
Diagnostics,
} from './types';
@@ -70,6 +69,51 @@ export function getDiagnosticHeader(opts: {
return markup`<filelink target="${filename}" line="${start.line}" column="${start.column}" />`;
}
+function removePositionFromLocation(loc: DiagnosticLocation): DiagnosticLocation {
+ return {
+ ...loc,
+ start: undefined,
+ end: undefined,
+ };
+}
+
+export function derivePositionlessKeyFromDiagnostic(diag: Diagnostic): string {
+ // Remove all line/column positions from the diagnostic and then JSON stringify it
+
+ let advice = diag.description.advice || [];
+
+ advice = advice.map((item) => {
+ switch (item.type) {
+ case 'frame':
+ return {
+ ...item,
+ location: removePositionFromLocation(item.location),
+ };
+
+ case 'action':
+ return {
+ ...item,
+ // Command flags could have position information
+ commandFlags: {},
+ };
+
+ default:
+ return item;
+ }
+ });
+
+ diag = {
+ ...diag,
+ description: {
+ ...diag.description,
+ advice,
+ },
+ location: removePositionFromLocation(diag.location),
+ };
+
+ return JSON.stringify(diag);
+}
+
export function deriveRootAdviceFromDiagnostic(diag: Diagnostic, opts: {
skipFrame: boolean;
includeHeaderInAdvice: boolean;

MODIFY
packages/@romejs/diagnostics/types.ts
packages/@romejs/diagnostics/types.ts
@@ -131,6 +131,7 @@ export type DiagnosticAdviceInspect = {
export type DiagnosticAdviceAction = {
type: 'action';
hidden?: boolean;
+ shortcut?: string;
instruction: string;
noun: string;
command: string;

MODIFY
packages/@romejs/js-compiler/lib/CompilerContext.ts
packages/@romejs/js-compiler/lib/CompilerContext.ts
@@ -270,7 +270,7 @@ export default class CompilerContext {
fixed?: New;
suggestions?: Array<{
description: string;
- short: string;
+ title: string;
fixed: New;
}>;
},
@@ -339,11 +339,13 @@ export default class CompilerContext {
// Add advice suggestions
let index = 0;
for (const suggestion of suggestions) {
+ const num = index + 1;
+
advice.push(
{
type: 'log',
- category: 'info',
- message: `Suggested fix #${index + 1}: <emphasis>${suggestion.description}</emphasis>`,
+ category: 'none',
+ message: `<emphasis>Suggested fix #${num}:</emphasis> ${suggestion.title}`,
},
);
@@ -354,6 +356,12 @@ export default class CompilerContext {
)),
});
+ advice.push({
+ type: 'log',
+ category: 'info',
+ message: suggestion.description,
+ });
+
if (loc === undefined) {
advice.push({
type: 'log',
@@ -362,7 +370,8 @@ export default class CompilerContext {
});
} else {
advice.push(buildLintDecisionAdviceAction({
- noun: `Apply "${suggestion.short}" fix`,
+ noun: `Apply suggestion "${suggestion.title}"`,
+ shortcut: String(num),
instruction: 'To apply this fix run',
filename: this.displayFilename,
action: 'fix',
@@ -414,6 +423,7 @@ export default class CompilerContext {
if (loc !== undefined && loc.start !== undefined) {
advice.push(buildLintDecisionAdviceAction({
noun: 'Add suppression comment',
+ shortcut: 's',
instruction: 'To suppress this error run',
filename: this.displayFilename,
action: 'suppress',

MODIFY
packages/@romejs/js-compiler/lib/Path.ts
packages/@romejs/js-compiler/lib/Path.ts
@@ -100,7 +100,7 @@ export default class Path {
const hook = this.findHook(descriptor, requiredDepth);
if (hook === undefined) {
if (optionalRet === undefined) {
- throw new Error('No hook found');
+ throw new Error(`No ${descriptor.name} hook found`);
} else {
return optionalRet;
}

MODIFY
packages/@romejs/js-compiler/lint/decisions.ts
packages/@romejs/js-compiler/lint/decisions.ts
@@ -76,8 +76,10 @@ export function buildLintDecisionAdviceAction({
action,
category,
start,
+ shortcut,
id,
}: {
+ shortcut?: string;
noun: string;
instruction: string;
filename: string;
@@ -104,11 +106,12 @@ export function buildLintDecisionAdviceAction({
type: 'action',
hidden: true,
command: 'lint',
+ shortcut,
args: [filename],
noun,
instruction,
commandFlags: {
- decision,
+ decisions: [decision],
},
};
}

MODIFY
packages/@romejs/js-compiler/lint/rules/camelCase.ts
packages/@romejs/js-compiler/lint/rules/camelCase.ts
@@ -77,10 +77,19 @@ export default {
const {name} = node;
const camelName = toVariableCamelCase(name);
if (camelName !== name) {
- return context.addFixableDiagnostic({
+ return context.addFixableDiagnostic(
+ {
old: node,
+ suggestions: [
+ {
+ title: 'Convert to camelCase',
+ description: 'This may not be safe. Are you passing this into a third party module?',
fixed: {...node, name: camelName},
- }, descriptions.LINT.IDENTIFIER_CAMEL_CASE(name, camelName));
+ },
+ ],
+ },
+ descriptions.LINT.IDENTIFIER_CAMEL_CASE(name, camelName),
+ );
}
}

MODIFY
packages/@romejs/js-compiler/lint/suppressions.ts
packages/@romejs/js-compiler/lint/suppressions.ts
@@ -99,8 +99,10 @@ export function addSuppressions(context: CompilerContext, ast: Program): Program
enter(path: Path) {
const {node} = path;
- // Don't allow attaching suppression comments to a comment...
- if (node.type === 'CommentBlock' || node.type === 'CommentLine') {
+ // Don't allow attaching suppression comments to a comment or program...
+ if (node.type === 'CommentBlock' || node.type === 'CommentLine' ||
+ node.type ===
+ 'Program') {
return node;
}

MODIFY
packages/@romejs/js-compiler/transforms/defaultHooks/index.ts
packages/@romejs/js-compiler/transforms/defaultHooks/index.ts
@@ -116,7 +116,7 @@ export const commentInjector = createHook<
CommentInjectorArg,
string
>({
- name: 'bindingInjectorHook',
+ name: 'commentInjectorHook',
initialState: {
comments: [],

MODIFY
packages/@romejs/vcs/index.ts
packages/@romejs/vcs/index.ts
@@ -59,7 +59,7 @@ function extractFileList(out: string): Array<string> {
const files: Array<string> = [];
for (const line of lines) {
- const match = line.match(/^[AM]\s+(.*?)$/);
+ const match = line.trim().match(/^(?:[AM]|\?\?)\s+(.*?)$/);
if (match != null) {
files.push(match[1]);
}

- Add shortcut to DiagnosticAdviceAction
- Fix git change extraction to work with padded lines and ?? prefixes
- Don't attach suppression comments to Program
- Turn identifier camelCase autofix to a suggestion as it's unsafe
- Make `--review` rerun the command after every action. Have some smartness to dedupe ignored diagnostics.