Skip to content

Commit 2eb9162

Browse files
72636c72636c
committed
Improve autofixing
1. Skip when ESLint reports no fixable issues 2. Detect ESLint warnings instead of just errors
1 parent 2fd3e9f commit 2eb9162

File tree

8 files changed

+136
-38
lines changed

8 files changed

+136
-38
lines changed

‎.changeset/six-worms-jump.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'skuba': patch
3+
---
4+
5+
lint: Detect and autofix ESLint warnings

‎.changeset/tasty-steaks-count.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'skuba': patch
3+
---
4+
5+
lint: Skip autofixing when ESLint reports no fixable issues

‎src/cli/adapter/eslint.ts

+8-2
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,10 @@ export interface ESLintResult {
2222

2323
export interface ESLintOutput {
2424
errors: ESLintResult[];
25-
warnings: ESLintResult[];
25+
fixable: boolean;
2626
ok: boolean;
2727
output: string;
28+
warnings: ESLintResult[];
2829
}
2930

3031
export const runESLint = async (
@@ -81,9 +82,14 @@ export const runESLint = async (
8182

8283
const errors: ESLintResult[] = [];
8384
const warnings: ESLintResult[] = [];
85+
let fixable = false;
8486

8587
for (const result of results) {
8688
const relativePath = path.relative(cwd, result.filePath);
89+
if (result.fixableErrorCount + result.fixableWarningCount) {
90+
fixable = true;
91+
}
92+
8793
if (result.errorCount) {
8894
errors.push({
8995
filePath: relativePath,
@@ -111,5 +117,5 @@ export const runESLint = async (
111117
logger.plain(output);
112118
}
113119

114-
return { ok, output, errors, warnings };
120+
return { errors, fixable, ok, output, warnings };
115121
};

‎src/cli/lint/annotate/github/eslint.test.ts

+4
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ it('should create failure annotations for ESLint errors', () => {
2424
],
2525
},
2626
],
27+
fixable: false,
2728
ok: false,
2829
output: '',
2930
warnings: [],
@@ -49,6 +50,7 @@ it('should create failure annotations for ESLint errors', () => {
4950
it('should create warning annotations for ESLint warnings', () => {
5051
const eslintOutput: ESLintOutput = {
5152
errors: [],
53+
fixable: false,
5254
ok: true,
5355
output: '',
5456
warnings: [
@@ -110,6 +112,7 @@ it('should create both failure and warning annotations for ESLint errors and war
110112
],
111113
},
112114
],
115+
fixable: false,
113116
ok: false,
114117
output: '',
115118
warnings: [
@@ -181,6 +184,7 @@ it('should not specify columns when an annotation spans multiple lines', () => {
181184
],
182185
},
183186
],
187+
fixable: false,
184188
ok: false,
185189
output: '',
186190
warnings: [],

‎src/cli/lint/annotate/github/index.test.ts

+1
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ const eslintOutput: ESLintOutput = {
3636
],
3737
},
3838
],
39+
fixable: false,
3940
ok: false,
4041
output: '',
4142
warnings: [],

‎src/cli/lint/autofix.test.ts

+78-14
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,10 @@ const stdout = () => {
2323
return `\n${result}`;
2424
};
2525

26-
const expectAutofixCommit = () => {
27-
expect(runESLint).toHaveBeenCalledTimes(1);
26+
const expectAutofixCommit = (
27+
{ eslint }: { eslint: boolean } = { eslint: true },
28+
) => {
29+
expect(runESLint).toHaveBeenCalledTimes(eslint ? 1 : 0);
2830
expect(runPrettier).toHaveBeenCalledTimes(1);
2931
expect(Git.commitAllChanges).toHaveBeenCalledTimes(1);
3032
};
@@ -51,26 +53,28 @@ beforeEach(() => {
5153
afterEach(jest.resetAllMocks);
5254

5355
describe('autofix', () => {
56+
const params = { debug: false, eslint: true, prettier: true };
57+
5458
it('bails on a non-CI environment', async () => {
5559
delete process.env.CI;
5660

57-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
61+
await expect(autofix(params)).resolves.toBeUndefined();
5862

5963
expectNoAutofix();
6064
});
6165

6266
it('bails on the master branch', async () => {
6367
jest.mocked(Git.currentBranch).mockResolvedValue('master');
6468

65-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
69+
await expect(autofix(params)).resolves.toBeUndefined();
6670

6771
expectNoAutofix();
6872
});
6973

7074
it('bails on the main branch', async () => {
7175
jest.mocked(Git.currentBranch).mockResolvedValue('main');
7276

73-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
77+
await expect(autofix(params)).resolves.toBeUndefined();
7478

7579
expectNoAutofix();
7680
});
@@ -80,7 +84,7 @@ describe('autofix', () => {
8084

8185
jest.mocked(Git.currentBranch).mockResolvedValue('devel');
8286

83-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
87+
await expect(autofix(params)).resolves.toBeUndefined();
8488

8589
expectNoAutofix();
8690
});
@@ -90,7 +94,7 @@ describe('autofix', () => {
9094

9195
jest.mocked(Git.currentBranch).mockResolvedValue('beta');
9296

93-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
97+
await expect(autofix(params)).resolves.toBeUndefined();
9498

9599
expectNoAutofix();
96100
});
@@ -101,15 +105,23 @@ describe('autofix', () => {
101105
.mocked(Git.getHeadCommitMessage)
102106
.mockResolvedValue('Run `skuba format`');
103107

104-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
108+
await expect(autofix(params)).resolves.toBeUndefined();
109+
110+
expectNoAutofix();
111+
});
112+
113+
it('bails on no fixable issues', async () => {
114+
await expect(
115+
autofix({ ...params, eslint: false, prettier: false }),
116+
).resolves.toBeUndefined();
105117

106118
expectNoAutofix();
107119
});
108120

109121
it('skips push on empty commit', async () => {
110122
jest.mocked(Git.commitAllChanges).mockResolvedValue(undefined);
111123

112-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
124+
await expect(autofix(params)).resolves.toBeUndefined();
113125

114126
expectAutofixCommit();
115127
expect(Git.push).not.toHaveBeenCalled();
@@ -131,7 +143,7 @@ describe('autofix', () => {
131143

132144
jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
133145

134-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
146+
await expect(autofix(params)).resolves.toBeUndefined();
135147

136148
expectAutofixCommit();
137149

@@ -151,7 +163,7 @@ describe('autofix', () => {
151163
jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
152164
jest.mocked(Git.currentBranch).mockResolvedValue('dev');
153165

154-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
166+
await expect(autofix(params)).resolves.toBeUndefined();
155167

156168
expectAutofixCommit();
157169
expect(Git.push).toHaveBeenNthCalledWith(1, {
@@ -170,6 +182,58 @@ describe('autofix', () => {
170182
`);
171183
});
172184

185+
it('handles fixable issues from ESLint only', async () => {
186+
jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
187+
jest.mocked(Git.currentBranch).mockResolvedValue('dev');
188+
189+
await expect(
190+
autofix({ ...params, eslint: true, prettier: false }),
191+
).resolves.toBeUndefined();
192+
193+
expectAutofixCommit();
194+
expect(Git.push).toHaveBeenNthCalledWith(1, {
195+
auth: { type: 'gitHubApp' },
196+
dir: expect.any(String),
197+
ref: 'commit-sha',
198+
remoteRef: 'dev',
199+
});
200+
201+
// We should run both ESLint and Prettier
202+
expect(stdout()).toMatchInlineSnapshot(`
203+
"
204+
205+
Trying to autofix with ESLint and Prettier...
206+
Pushed fix commit commit-sha.
207+
"
208+
`);
209+
});
210+
211+
it('handles fixable issues from Prettier only', async () => {
212+
jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
213+
jest.mocked(Git.currentBranch).mockResolvedValue('dev');
214+
215+
await expect(
216+
autofix({ ...params, eslint: false, prettier: true }),
217+
).resolves.toBeUndefined();
218+
219+
expectAutofixCommit({ eslint: false });
220+
expect(Git.push).toHaveBeenNthCalledWith(1, {
221+
auth: { type: 'gitHubApp' },
222+
dir: expect.any(String),
223+
ref: 'commit-sha',
224+
remoteRef: 'dev',
225+
});
226+
227+
// We should only run Prettier
228+
expect(stdout()).toMatchInlineSnapshot(`
229+
"
230+
231+
Trying to autofix with Prettier...
232+
Pushed fix commit commit-sha.
233+
"
234+
`);
235+
});
236+
173237
it('tolerates guard errors', async () => {
174238
const ERROR = new Error('badness!');
175239

@@ -178,7 +242,7 @@ describe('autofix', () => {
178242

179243
jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
180244

181-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
245+
await expect(autofix(params)).resolves.toBeUndefined();
182246

183247
expectAutofixCommit();
184248
expect(Git.push).toHaveBeenNthCalledWith(1, {
@@ -199,7 +263,7 @@ describe('autofix', () => {
199263
it('bails on commit error', async () => {
200264
jest.mocked(Git.commitAllChanges).mockRejectedValue(MOCK_ERROR);
201265

202-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
266+
await expect(autofix(params)).resolves.toBeUndefined();
203267

204268
expectAutofixCommit();
205269
expect(Git.push).not.toHaveBeenCalled();
@@ -219,7 +283,7 @@ describe('autofix', () => {
219283
jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha');
220284
jest.mocked(Git.push).mockRejectedValue(MOCK_ERROR);
221285

222-
await expect(autofix({ debug: false })).resolves.toBeUndefined();
286+
await expect(autofix(params)).resolves.toBeUndefined();
223287

224288
expectAutofixCommit();
225289
expect(Git.push).toHaveBeenNthCalledWith(1, {

‎src/cli/lint/autofix.ts

+21-5
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,18 @@ const shouldPush = async ({
5656
return true;
5757
};
5858

59-
export const autofix = async (input: Pick<Input, 'debug'>): Promise<void> => {
59+
interface AutofixParameters {
60+
debug: Input['debug'];
61+
62+
eslint: boolean;
63+
prettier: boolean;
64+
}
65+
66+
export const autofix = async (params: AutofixParameters): Promise<void> => {
67+
if (!params.eslint && !params.prettier) {
68+
return;
69+
}
70+
6071
const dir = process.cwd();
6172

6273
let currentBranch;
@@ -68,14 +79,19 @@ export const autofix = async (input: Pick<Input, 'debug'>): Promise<void> => {
6879
return;
6980
}
7081

71-
// Naively try to autofix issues as we can't tell from ESLint output.
7282
try {
7383
log.newline();
74-
log.warn(`Trying to autofix with ESLint and Prettier...`);
84+
log.warn(
85+
`Trying to autofix with ${params.eslint ? 'ESLint and ' : ''}Prettier...`,
86+
);
7587

76-
const logger = createLogger(input.debug);
88+
const logger = createLogger(params.debug);
7789

78-
await runESLint('format', logger);
90+
if (params.eslint) {
91+
await runESLint('format', logger);
92+
}
93+
// Unconditionally re-run Prettier; reaching here means we have pre-existing
94+
// format violations or may have created new ones through ESLint fixes.
7995
await runPrettier('format', logger);
8096

8197
const ref = await Git.commitAllChanges({

‎src/cli/lint/external.ts

+14-17
Original file line numberDiff line numberDiff line change
@@ -96,25 +96,22 @@ export const externalLint = async (input: Input) => {
9696
log.subtle(inspect(err));
9797
}
9898

99-
if (eslint.ok && prettier.ok && tscOk) {
100-
return;
101-
}
102-
103-
const tools = [
104-
...(eslint.ok ? [] : ['ESLint']),
105-
...(prettier.ok ? [] : ['Prettier']),
106-
...(tscOk ? [] : ['tsc']),
107-
];
108-
109-
log.newline();
110-
log.err(`${tools.join(', ')} found issues that require triage.`);
99+
if (!eslint.ok || !prettier.ok || !tscOk) {
100+
const tools = [
101+
...(eslint.ok ? [] : ['ESLint']),
102+
...(prettier.ok ? [] : ['Prettier']),
103+
...(tscOk ? [] : ['tsc']),
104+
];
111105

112-
process.exitCode = 1;
106+
log.newline();
107+
log.err(`${tools.join(', ')} found issues that require triage.`);
113108

114-
if (eslint.ok && prettier.ok) {
115-
// If these are fine then the issue lies with tsc, which we can't autofix.
116-
return;
109+
process.exitCode = 1;
117110
}
118111

119-
await autofix(input);
112+
await autofix({
113+
debug: input.debug,
114+
eslint: eslint.fixable,
115+
prettier: !prettier.ok,
116+
});
120117
};

0 commit comments

Comments
 (0)