Skip to content

Commit 33789ea

Browse files
EvilGenius13EvilGenius13
authored
Merge pull request #1043 from Shopify/update-wording-for-conditional-check
Reworded error messages for conditional html liquid syntax check
2 parents 5ff6277 + 2784a83 commit 33789ea

File tree

3 files changed

+62
-26
lines changed

3 files changed

+62
-26
lines changed

‎.changeset/tough-ties-tickle.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme-check-common': patch
3+
---
4+
5+
Change wording around syntax errors for invalid conditional nodes in Liquid HTML Syntax check

‎packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidConditionalNode.spec.ts‎

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
4747

4848
expect(offenses).to.have.length(1);
4949
expect(offenses[0].message).to.equal(
50-
"Liquid lax parsing issue: Expression stops at truthy value '7', ignoring: '1 > 100'",
50+
"Syntax is not supported: Expression stops at truthy value '7', and will ignore: '1 > 100'",
5151
);
5252

5353
const fixed = applyFix(source, offenses[0]);
@@ -60,7 +60,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
6060

6161
expect(offenses).to.have.length(1);
6262
expect(offenses[0].message).to.equal(
63-
"Liquid lax parsing issue: Expression stops at truthy value ''hello'', ignoring: '1 > 100'",
63+
"Syntax is not supported: Expression stops at truthy value ''hello'', and will ignore: '1 > 100'",
6464
);
6565

6666
const fixed = applyFix(source, offenses[0]);
@@ -108,7 +108,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
108108

109109
expect(offenses).to.have.length(1);
110110
expect(offenses[0].message).to.equal(
111-
"Liquid lax parsing issue: Malformed expression starting with invalid token '>'",
111+
"Syntax is not supported: Conditional cannot start with '>'. Use a variable or value instead",
112112
);
113113

114114
const fixed = applyFix(source, offenses[0]);
@@ -128,9 +128,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
128128
for (const testCase of testCases) {
129129
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, testCase.source);
130130
expect(offenses).to.have.length(1);
131-
expect(offenses[0].message).to.contain(
132-
`Malformed expression starting with invalid token '${testCase.token}'`,
133-
);
131+
expect(offenses[0].message).to.contain(`Conditional cannot start with '${testCase.token}'`);
134132

135133
const fixed = applyFix(testCase.source, offenses[0]);
136134
expect(fixed).to.equal('{% if false %}hello{% endif %}');
@@ -148,9 +146,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
148146
for (const testCase of testCases) {
149147
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, testCase.source);
150148
expect(offenses).to.have.length(1);
151-
expect(offenses[0].message).to.contain(
152-
`Malformed expression starting with invalid token '${testCase.token}'`,
153-
);
149+
expect(offenses[0].message).to.contain(`Conditional cannot start with '${testCase.token}'`);
154150

155151
const fixed = applyFix(testCase.source, offenses[0]);
156152
expect(fixed).to.equal('{% if false %}hello{% endif %}');
@@ -167,7 +163,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
167163
for (const testCase of testCases) {
168164
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, testCase);
169165
expect(offenses).to.have.length(1);
170-
expect(offenses[0].message).to.contain('Malformed expression starting with invalid token');
166+
expect(offenses[0].message).to.contain('Conditional cannot start with');
171167

172168
const fixed = applyFix(testCase, offenses[0]);
173169
expect(fixed).to.equal('{% if false %}hello{% endif %}');
@@ -180,7 +176,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
180176

181177
expect(offenses).to.have.length(1);
182178
expect(offenses[0].message).to.equal(
183-
"Liquid lax parsing issue: Trailing tokens ignored after comparison: 'foobar'",
179+
"Syntax is not supported: Conditional is invalid. Anything after '1 == 2' will be ignored",
184180
);
185181

186182
const fixed = applyFix(source, offenses[0]);
@@ -206,7 +202,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
206202
for (const testCase of testCases) {
207203
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, testCase.source);
208204
expect(offenses, `Failed for: ${testCase.description}`).to.have.length(1);
209-
expect(offenses[0].message).to.contain('Trailing tokens ignored');
205+
expect(offenses[0].message).to.contain('Anything after');
210206
}
211207
});
212208

@@ -215,7 +211,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
215211
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, source);
216212

217213
expect(offenses).to.have.length(1);
218-
expect(offenses[0].message).to.contain('Trailing tokens ignored');
214+
expect(offenses[0].message).to.contain('Anything after');
219215

220216
const fixed = applyFix(source, offenses[0]);
221217
expect(fixed).to.equal('{% if 10 > 4 %}hello{% endif %}');
@@ -231,7 +227,7 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
231227
for (const testCase of testCases) {
232228
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, testCase);
233229
expect(offenses).to.have.length(1);
234-
expect(offenses[0].message).to.contain('Trailing tokens ignored');
230+
expect(offenses[0].message).to.contain('Anything after');
235231
}
236232
});
237233

@@ -351,13 +347,30 @@ describe('Module: InvalidConditionalBooleanExpression', () => {
351347
for (const testCase of testCases) {
352348
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, testCase.source);
353349
expect(offenses).to.have.length(1);
354-
expect(offenses[0].message).to.contain('Trailing tokens ignored after comparison');
350+
expect(offenses[0].message).to.contain('Anything after');
355351

356352
const fixed = applyFix(testCase.source, offenses[0]);
357353
expect(fixed).to.contain(testCase.expectedFix);
358354
}
359355
});
360356

357+
it('should report special message for JavaScript-style operators after literal values', async () => {
358+
const testCases = [
359+
'{% if true && false %}hello{% endif %}',
360+
'{% if false || true %}hello{% endif %}',
361+
'{% if "hello" && world %}hello{% endif %}',
362+
'{% if 42 || something %}hello{% endif %}',
363+
];
364+
365+
for (const source of testCases) {
366+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, source);
367+
expect(offenses).to.have.length(1);
368+
expect(offenses[0].message).to.contain(
369+
"Use 'and'/'or' instead of '&&'/'||' for multiple conditions",
370+
);
371+
}
372+
});
373+
361374
it('should NOT report an offense for valid logical operators', async () => {
362375
const validCases = [
363376
'{% if price > 100 and discount < 50 %}hello{% endif %}',

‎packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidConditionalNode.ts‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { LiquidBranch, LiquidTag } from '@shopify/liquid-html-parser';
22
import { SourceCodeType, Problem } from '../../..';
3-
import { getValuesInMarkup } from './utils';
3+
import { getValuesInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
44

55
type TokenType = 'variable' | keyof typeof TOKEN_PATTERNS;
66

@@ -51,7 +51,7 @@ export function detectInvalidConditionalNode(
5151
const endIndex = startIndex + markup.length;
5252

5353
return {
54-
message: `Liquid lax parsing issue: ${issue.message}`,
54+
message: `${INVALID_SYNTAX_MESSAGE}: ${issue.message}`,
5555
startIndex,
5656
endIndex,
5757
fix: (corrector) => {
@@ -72,7 +72,7 @@ function checkInvalidStartingToken(tokens: Token[]): ExpressionIssue | null {
7272
const firstToken = tokens[0];
7373
if (firstToken.type === 'invalid' || firstToken.type === 'comparison') {
7474
return {
75-
message: `Malformed expression starting with invalid token '${firstToken.value}'`,
75+
message: `Conditional cannot start with '${firstToken.value}'. Use a variable or value instead`,
7676
fix: 'false',
7777
};
7878
}
@@ -95,10 +95,19 @@ function checkTrailingTokensAfterComparison(tokens: Token[]): ExpressionIssue |
9595
.map((t) => t.value)
9696
.join(' ');
9797
const junk = remaining.map((t) => t.value).join(' ');
98-
return {
99-
message: `Trailing tokens ignored after comparison: '${junk.trim()}'`,
100-
fix: validExpr,
101-
};
98+
const containsLogicalOperators = /&&|\|\|/.test(junk);
99+
100+
if (containsLogicalOperators) {
101+
return {
102+
message: `Conditional is invalid. Anything after '${validExpr}' will be ignored. Use 'and'/'or' instead of '&&'/'||' for multiple conditions`,
103+
fix: validExpr,
104+
};
105+
} else {
106+
return {
107+
message: `Conditional is invalid. Anything after '${validExpr}' will be ignored`,
108+
fix: validExpr,
109+
};
110+
}
102111
}
103112
}
104113
}
@@ -118,10 +127,19 @@ function checkLaxParsingIssues(tokens: Token[]): ExpressionIssue | null {
118127

119128
if (!hasUnknownOperator) {
120129
const ignored = remaining.map((t) => t.value).join(' ');
121-
return {
122-
message: `Expression stops at truthy value '${current.value}', ignoring: '${ignored}'`,
123-
fix: current.value,
124-
};
130+
const containsLogicalOperators = /&&|\|\|/.test(ignored);
131+
132+
if (containsLogicalOperators) {
133+
return {
134+
message: `Expression stops at truthy value '${current.value}', and will ignore: '${ignored}'. Use 'and'/'or' instead of '&&'/'||' for multiple conditions`,
135+
fix: current.value,
136+
};
137+
} else {
138+
return {
139+
message: `Expression stops at truthy value '${current.value}', and will ignore: '${ignored}'`,
140+
fix: current.value,
141+
};
142+
}
125143
}
126144
}
127145
}

0 commit comments

Comments
 (0)