Skip to content

Commit b0e517b

Browse files
shoplandoshoplando
committed
add code actions for disabling theme-check rules
1 parent c9fe538 commit b0e517b

File tree

5 files changed

+384
-2
lines changed

5 files changed

+384
-2
lines changed

‎.changeset/odd-rice-return.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme-language-server-common': minor
3+
---
4+
5+
Added Disable check option in the quick fix menu to disable the theme-check warning/error either for just that line or for the entire file

‎packages/theme-language-server-common/src/codeActions/CodeActionsProvider.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,24 @@
11
import { CodeAction, CodeActionParams, Command } from 'vscode-languageserver';
22
import { DiagnosticsManager } from '../diagnostics';
33
import { DocumentManager } from '../documents';
4-
import { FixAllProvider, FixProvider, SuggestionProvider } from './providers';
4+
import { FixAllProvider, FixProvider, SuggestionProvider, DisableCheckProvider } from './providers';
55
import { BaseCodeActionsProvider } from './BaseCodeActionsProvider';
66

77
export const CodeActionKinds = Array.from(
8-
new Set([FixAllProvider.kind, FixProvider.kind, SuggestionProvider.kind]),
8+
new Set([
9+
DisableCheckProvider.kind,
10+
FixAllProvider.kind,
11+
FixProvider.kind,
12+
SuggestionProvider.kind,
13+
]),
914
);
1015

1116
export class CodeActionsProvider {
1217
private providers: BaseCodeActionsProvider[];
1318

1419
constructor(documentManager: DocumentManager, diagnosticsManager: DiagnosticsManager) {
1520
this.providers = [
21+
new DisableCheckProvider(documentManager, diagnosticsManager),
1622
new FixAllProvider(documentManager, diagnosticsManager),
1723
new FixProvider(documentManager, diagnosticsManager),
1824
new SuggestionProvider(documentManager, diagnosticsManager),

‎packages/theme-language-server-common/src/codeActions/providers/DisableCheckProvider.spec.ts‎

Lines changed: 267 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,267 @@
1+
import { describe, it, expect, vi, beforeEach } from 'vitest';
2+
import { URI } from 'vscode-uri';
3+
import { Offense, SourceCodeType, Severity, path } from '@shopify/theme-check-common';
4+
import { DiagnosticsManager } from '../../diagnostics';
5+
import { DocumentManager } from '../../documents';
6+
import { DisableCheckProvider } from './DisableCheckProvider';
7+
import { TextDocument } from 'vscode-languageserver-textdocument';
8+
9+
describe('Unit: DisableCheckProvider', () => {
10+
const liquidUri = path.normalize(URI.file('/path/to/file.liquid'));
11+
const liquidContents = `
12+
{% assign x = 1 %}
13+
<script src="2.js"></script>
14+
<script src="3.js"></script>
15+
`;
16+
const liquidDocument = TextDocument.create(liquidUri, 'liquid', 0, liquidContents);
17+
let documentManager: DocumentManager;
18+
let diagnosticsManager: DiagnosticsManager;
19+
let disableCheckProvider: DisableCheckProvider;
20+
21+
function makeOffense(checkName: string, needle: string): Offense<SourceCodeType.LiquidHtml> {
22+
const start = liquidContents.indexOf(needle);
23+
const end = start + needle.length;
24+
25+
const messages: Record<string, string> = {
26+
UnusedAssign: `Variable 'x' is defined but not used`,
27+
ParserBlockingScript: 'Parser blocking script detected',
28+
};
29+
30+
return {
31+
type: SourceCodeType.LiquidHtml,
32+
check: checkName,
33+
message: messages[checkName] || 'Offense detected',
34+
uri: liquidUri,
35+
severity: Severity.ERROR,
36+
start: { ...liquidDocument.positionAt(start), index: start },
37+
end: { ...liquidDocument.positionAt(end), index: end },
38+
};
39+
}
40+
41+
beforeEach(() => {
42+
documentManager = new DocumentManager();
43+
diagnosticsManager = new DiagnosticsManager({ sendDiagnostics: vi.fn() } as any);
44+
disableCheckProvider = new DisableCheckProvider(documentManager, diagnosticsManager);
45+
});
46+
47+
describe('Liquid files', () => {
48+
beforeEach(() => {
49+
documentManager.open(liquidUri, liquidContents, 1);
50+
});
51+
52+
describe('When single offense exists under cursor', () => {
53+
beforeEach(() => {
54+
diagnosticsManager.set(liquidUri, 1, [makeOffense('UnusedAssign', '{% assign x = 1 %}')]);
55+
});
56+
57+
it('provides disable actions for the offense', () => {
58+
const codeActions = disableCheckProvider.codeActions({
59+
textDocument: { uri: liquidUri },
60+
range: {
61+
start: liquidDocument.positionAt(liquidContents.indexOf('assign')),
62+
end: liquidDocument.positionAt(liquidContents.indexOf('assign')),
63+
},
64+
context: { diagnostics: [] },
65+
});
66+
67+
expect(codeActions.length).toBe(2);
68+
69+
const [disableNextLineAction, disableFileAction] = codeActions;
70+
71+
expect(disableNextLineAction).toEqual({
72+
title: 'Disable UnusedAssign for this line',
73+
kind: 'quickfix',
74+
diagnostics: expect.any(Array),
75+
isPreferred: false,
76+
command: {
77+
title: 'Disable UnusedAssign for this line',
78+
command: 'themeCheck/applyDisableCheck',
79+
arguments: [liquidUri, 1, 'next-line', 'UnusedAssign', 1],
80+
},
81+
});
82+
83+
expect(disableFileAction).toEqual({
84+
title: 'Disable UnusedAssign for entire file',
85+
kind: 'quickfix',
86+
diagnostics: expect.any(Array),
87+
isPreferred: false,
88+
command: {
89+
title: 'Disable UnusedAssign for entire file',
90+
command: 'themeCheck/applyDisableCheck',
91+
arguments: [liquidUri, 1, 'file', 'UnusedAssign', 0],
92+
},
93+
});
94+
});
95+
});
96+
97+
describe('When multiple offenses of same type exist on same line', () => {
98+
beforeEach(() => {
99+
diagnosticsManager.set(liquidUri, 1, [
100+
makeOffense('ParserBlockingScript', '<script'),
101+
makeOffense('ParserBlockingScript', 'src="2.js"'),
102+
]);
103+
});
104+
105+
it('provides only one disable action per check type per line', () => {
106+
const codeActions = disableCheckProvider.codeActions({
107+
textDocument: { uri: liquidUri },
108+
range: {
109+
start: liquidDocument.positionAt(liquidContents.indexOf('<script')),
110+
end: liquidDocument.positionAt(
111+
liquidContents.indexOf('</script>') + '</script>'.length,
112+
),
113+
},
114+
context: { diagnostics: [] },
115+
});
116+
117+
expect(codeActions.length).toBe(2);
118+
119+
const disableNextLineActions = codeActions.filter(
120+
(action) =>
121+
typeof action.command === 'object' && action.command?.arguments?.[2] === 'next-line',
122+
);
123+
expect(disableNextLineActions.length).toBe(1);
124+
expect(disableNextLineActions[0].title).toBe('Disable ParserBlockingScript for this line');
125+
});
126+
});
127+
128+
describe('When multiple offenses of various types exist', () => {
129+
beforeEach(() => {
130+
diagnosticsManager.set(liquidUri, 1, [
131+
makeOffense('UnusedAssign', '{% assign x = 1 %}'),
132+
makeOffense('ParserBlockingScript', '<script src="2.js"></script>'),
133+
makeOffense('ParserBlockingScript', '<script src="3.js"></script>'),
134+
]);
135+
});
136+
137+
it('provides disable actions for each unique check under cursor', () => {
138+
const codeActions = disableCheckProvider.codeActions({
139+
textDocument: { uri: liquidUri },
140+
range: {
141+
start: liquidDocument.positionAt(0),
142+
end: liquidDocument.positionAt(liquidContents.length),
143+
},
144+
context: { diagnostics: [] },
145+
});
146+
147+
// Should have 2 checks × 2 action types = 4 actions
148+
// But duplicates on same line are filtered
149+
const uniqueActions = new Set(
150+
codeActions
151+
.map((a) => {
152+
if (typeof a.command === 'object' && a.command?.arguments) {
153+
return `${a.command.arguments[3]}-${a.command.arguments[2]}`;
154+
}
155+
return '';
156+
})
157+
.filter(Boolean),
158+
);
159+
160+
expect(uniqueActions.has('UnusedAssign-next-line')).toBe(true);
161+
expect(uniqueActions.has('UnusedAssign-file')).toBe(true);
162+
expect(uniqueActions.has('ParserBlockingScript-next-line')).toBe(true);
163+
expect(uniqueActions.has('ParserBlockingScript-file')).toBe(true);
164+
});
165+
});
166+
167+
describe('When cursor is not on any offense', () => {
168+
beforeEach(() => {
169+
diagnosticsManager.set(liquidUri, 1, [makeOffense('UnusedAssign', '{% assign x = 1 %}')]);
170+
});
171+
172+
it('provides no code actions', () => {
173+
const codeActions = disableCheckProvider.codeActions({
174+
textDocument: { uri: liquidUri },
175+
range: {
176+
start: liquidDocument.positionAt(liquidContents.length - 1),
177+
end: liquidDocument.positionAt(liquidContents.length),
178+
},
179+
context: { diagnostics: [] },
180+
});
181+
182+
expect(codeActions).toEqual([]);
183+
});
184+
});
185+
186+
describe('When inside stylesheet or javascript tags', () => {
187+
it('provides no code actions inside stylesheet tags', () => {
188+
const stylesheetContents = `
189+
{% stylesheet %}
190+
@starting-style {
191+
opacity: 0;
192+
}
193+
{% endstylesheet %}
194+
`;
195+
const stylesheetDocument = TextDocument.create(liquidUri, 'liquid', 1, stylesheetContents);
196+
documentManager.open(liquidUri, stylesheetContents, 1);
197+
198+
// Create a CSS-related offense inside the stylesheet tag
199+
const errorText = '@starting-style';
200+
const errorStart = stylesheetContents.indexOf(errorText);
201+
const errorEnd = errorStart + errorText.length;
202+
203+
diagnosticsManager.set(liquidUri, 1, [
204+
{
205+
type: SourceCodeType.LiquidHtml,
206+
check: 'CSSCheck',
207+
message: 'Unknown at rule @starting-style',
208+
uri: liquidUri,
209+
severity: Severity.ERROR,
210+
start: { ...stylesheetDocument.positionAt(errorStart), index: errorStart },
211+
end: { ...stylesheetDocument.positionAt(errorEnd), index: errorEnd },
212+
},
213+
]);
214+
215+
const codeActions = disableCheckProvider.codeActions({
216+
textDocument: { uri: liquidUri },
217+
range: {
218+
start: stylesheetDocument.positionAt(errorStart),
219+
end: stylesheetDocument.positionAt(errorEnd),
220+
},
221+
context: { diagnostics: [] },
222+
});
223+
224+
expect(codeActions).toEqual([]);
225+
});
226+
227+
it('provides no code actions inside javascript tags', () => {
228+
const javascriptContents = `
229+
{% javascript %}
230+
console.log('error');
231+
const x = await fetch('/api');
232+
{% endjavascript %}
233+
`;
234+
const javascriptDocument = TextDocument.create(liquidUri, 'liquid', 1, javascriptContents);
235+
documentManager.open(liquidUri, javascriptContents, 1);
236+
237+
// Create a JavaScript-related offense inside the javascript tag
238+
const errorText = 'await';
239+
const errorStart = javascriptContents.indexOf(errorText);
240+
const errorEnd = errorStart + errorText.length;
241+
242+
diagnosticsManager.set(liquidUri, 1, [
243+
{
244+
type: SourceCodeType.LiquidHtml,
245+
check: 'JSCheck',
246+
message: 'await is only valid in async functions',
247+
uri: liquidUri,
248+
severity: Severity.ERROR,
249+
start: { ...javascriptDocument.positionAt(errorStart), index: errorStart },
250+
end: { ...javascriptDocument.positionAt(errorEnd), index: errorEnd },
251+
},
252+
]);
253+
254+
const codeActions = disableCheckProvider.codeActions({
255+
textDocument: { uri: liquidUri },
256+
range: {
257+
start: javascriptDocument.positionAt(errorStart),
258+
end: javascriptDocument.positionAt(errorEnd),
259+
},
260+
context: { diagnostics: [] },
261+
});
262+
263+
expect(codeActions).toEqual([]);
264+
});
265+
});
266+
});
267+
});

0 commit comments

Comments
 (0)