Skip to content

Commit 1b5f67a

Browse files
markokajzermarkokajzer
authored
Linter: Implement erb-no-conditional-open-tag rule (#1200)
Closes #1157
1 parent 5c6a51f commit 1b5f67a

File tree

6 files changed

+346
-3
lines changed

6 files changed

+346
-3
lines changed

‎javascript/packages/linter/docs/rules/erb-no-conditional-open-tag.md‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
# Linter Rule: Disallow conditional open tags
2+
3+
**Rule:** `erb-no-conditional-open-tag`
4+
5+
## Description
6+
7+
Disallow the pattern of using ERB conditionals to vary only the open tag of an element (typically to conditionally change attributes) while sharing the same tag name, body content, and close tag.
8+
9+
## Rationale
10+
11+
This pattern can be difficult to read and maintain. The conditional logic is split from the element's body and closing tag, making it harder to understand the full structure at a glance.
12+
13+
Prefer using:
14+
15+
- `content_tag` with conditional attributes
16+
- A ternary or conditional for the class/attribute value directly
17+
- Separate complete elements in each branch if the differences are significant
18+
19+
## Examples
20+
21+
### ✅ Good
22+
23+
Using conditional attributes directly:
24+
25+
```erb
26+
<div class="<%= @condition ? 'a' : 'b' %>">
27+
Content
28+
</div>
29+
```
30+
31+
Using `content_tag` with conditional options:
32+
33+
```erb
34+
<%= content_tag :div, class: (@condition ? 'a' : 'b') do %>
35+
Content
36+
<% end %>
37+
```
38+
39+
Using `class_names` helper:
40+
41+
```erb
42+
<div class="<%= class_names('base', 'a': @condition, 'b': !@condition) %>">
43+
Content
44+
</div>
45+
```
46+
47+
Complete separate elements when differences are significant:
48+
49+
```erb
50+
<% if @condition %>
51+
<div class="a" data-foo="bar">Content</div>
52+
<% else %>
53+
<div class="b" data-baz="qux">Content</div>
54+
<% end %>
55+
```
56+
57+
Self-closing/void elements in conditionals (each branch is complete):
58+
59+
```erb
60+
<% if @large %>
61+
<img src="photo.jpg" width="800" height="600">
62+
<% else %>
63+
<img src="photo.jpg" width="400" height="300">
64+
<% end %>
65+
```
66+
67+
### 🚫 Bad
68+
69+
Conditional open tags with shared body and close tag:
70+
71+
```erb
72+
<% if @condition %>
73+
<div class="a">
74+
<% else %>
75+
<div class="b">
76+
<% end %>
77+
Content
78+
</div>
79+
```
80+
81+
```erb
82+
<% if @with_icon %>
83+
<button class="btn btn-icon" aria-label="Action">
84+
<% else %>
85+
<button class="btn">
86+
<% end %>
87+
Click me
88+
</button>
89+
```
90+
91+
Open tag in conditional without else branch:
92+
93+
```erb
94+
<% if @wrap %>
95+
<div class="wrapper">
96+
<% end %>
97+
Content
98+
</div>
99+
```
100+
101+
Missing open tag in else branch:
102+
103+
```erb
104+
<% if @style == "a" %>
105+
<div class="a">
106+
<% elsif @style == "b" %>
107+
<div class="b">
108+
<% else %>
109+
<% end %>
110+
Content
111+
</div>
112+
```
113+
114+
Missing open tag in elsif branch:
115+
116+
```erb
117+
<% if @style == "a" %>
118+
<div class="a">
119+
<% elsif @style == "b" %>
120+
<%# no-open-tag %>
121+
<% else %>
122+
<div class="c">
123+
<% end %>
124+
Content
125+
</div>
126+
```
127+
128+
## References
129+
130+
\-

‎javascript/packages/linter/src/rules.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { RuleClass } from "./types.js"
33
import { ERBCommentSyntax } from "./rules/erb-comment-syntax.js";
44
import { ERBNoCaseNodeChildrenRule } from "./rules/erb-no-case-node-children.js"
55
import { ERBNoConditionalHTMLElementRule } from "./rules/erb-no-conditional-html-element.js"
6+
import { ERBNoConditionalOpenTagRule } from "./rules/erb-no-conditional-open-tag.js"
67
import { ERBNoEmptyTagsRule } from "./rules/erb-no-empty-tags.js"
78
import { ERBNoExtraNewLineRule } from "./rules/erb-no-extra-newline.js"
89
import { ERBNoExtraWhitespaceRule } from "./rules/erb-no-extra-whitespace-inside-tags.js"
@@ -65,6 +66,7 @@ export const rules: RuleClass[] = [
6566
ERBCommentSyntax,
6667
ERBNoCaseNodeChildrenRule,
6768
ERBNoConditionalHTMLElementRule,
69+
ERBNoConditionalOpenTagRule,
6870
ERBNoEmptyTagsRule,
6971
ERBNoExtraNewLineRule,
7072
ERBNoExtraWhitespaceRule,

‎javascript/packages/linter/src/rules/erb-no-conditional-open-tag.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import dedent from "dedent"
2+
3+
import { ParserRule } from "../types.js"
4+
import { BaseRuleVisitor } from "./rule-utils.js"
5+
6+
import type { ParseResult, HTMLConditionalOpenTagNode } from "@herb-tools/core"
7+
import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js"
8+
9+
class ERBNoConditionalOpenTagRuleVisitor extends BaseRuleVisitor {
10+
visitHTMLConditionalOpenTagNode(node: HTMLConditionalOpenTagNode): void {
11+
const tagName = node.tag_name?.value || "element"
12+
13+
this.addOffense(
14+
`Avoid using ERB conditionals to split the open and closing tag of \`<${tagName}>\` element.`,
15+
node.location,
16+
)
17+
18+
this.visitChildNodes(node)
19+
}
20+
}
21+
22+
export class ERBNoConditionalOpenTagRule extends ParserRule {
23+
name = "erb-no-conditional-open-tag"
24+
25+
get defaultConfig(): FullRuleConfig {
26+
return {
27+
enabled: true,
28+
severity: "error"
29+
}
30+
}
31+
32+
check(result: ParseResult, context?: Partial<LintContext>): UnboundLintOffense[] {
33+
const visitor = new ERBNoConditionalOpenTagRuleVisitor(this.name, context)
34+
35+
visitor.visit(result.value)
36+
37+
return visitor.offenses
38+
}
39+
}

‎javascript/packages/linter/src/rules/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ export * from "./herb-disable-comment-base.js"
55

66
export * from "./erb-comment-syntax.js"
77
export * from "./erb-no-case-node-children.js"
8+
export * from "./erb-no-conditional-open-tag.js"
89
export * from "./erb-no-empty-tags.js"
910
export * from "./erb-no-extra-newline.js"
1011
export * from "./erb-no-extra-whitespace-inside-tags.js"

‎javascript/packages/linter/test/__snapshots__/cli.test.ts.snap‎

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎javascript/packages/linter/test/rules/erb-no-conditional-open-tag.test.ts‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
import { describe, it } from "vitest"
2+
import dedent from "dedent";
3+
4+
import { ERBNoConditionalOpenTagRule } from "../../src/rules/erb-no-conditional-open-tag.js";
5+
import { createLinterTest } from "../helpers/linter-test-helper.js"
6+
7+
const { expectNoOffenses, expectError, assertOffenses } = createLinterTest(ERBNoConditionalOpenTagRule)
8+
9+
const errorMessage = (tagName: string) =>
10+
`Avoid using ERB conditionals to split the open and closing tag of \`<${tagName}>\` element.`
11+
12+
describe("erb-no-conditional-open-tag", () => {
13+
it("allows conditional attributes directly", () => {
14+
const html = dedent`
15+
<div class="<%= @condition ? 'a' : 'b' %>">
16+
Content
17+
</div>
18+
`
19+
20+
expectNoOffenses(html)
21+
})
22+
23+
it("allows content_tag with conditional options", () => {
24+
const html = dedent`
25+
<%= content_tag :div, class: (@condition ? 'a' : 'b') do %>
26+
Content
27+
<% end %>
28+
`
29+
30+
expectNoOffenses(html)
31+
})
32+
33+
it("allows class_names helper", () => {
34+
const html = dedent`
35+
<div class="<%= class_names('base', 'a': @condition, 'b': !@condition) %>">
36+
Content
37+
</div>
38+
`
39+
40+
expectNoOffenses(html)
41+
})
42+
43+
it("allows complete separate elements in each branch", () => {
44+
const html = dedent`
45+
<% if @condition %>
46+
<div class="a" data-foo="bar">Content</div>
47+
<% else %>
48+
<div class="b" data-baz="qux">Content</div>
49+
<% end %>
50+
`
51+
52+
expectNoOffenses(html)
53+
})
54+
55+
it("allows void elements in conditionals", () => {
56+
const html = dedent`
57+
<% if @large %>
58+
<img src="photo.jpg" width="800" height="600">
59+
<% else %>
60+
<img src="photo.jpg" width="400" height="300">
61+
<% end %>
62+
`
63+
64+
expectNoOffenses(html)
65+
})
66+
67+
it("allows normal if statements wrapping complete elements", () => {
68+
const html = dedent`
69+
<% if true %>
70+
<div>Text</div>
71+
<% end %>
72+
`
73+
74+
expectNoOffenses(html)
75+
})
76+
77+
it("allows regular HTML elements", () => {
78+
const html = dedent`
79+
<div class="container">
80+
<p>Content</p>
81+
</div>
82+
`
83+
84+
expectNoOffenses(html)
85+
})
86+
87+
it("reports conditional open tag with if/else varying class", () => {
88+
const html = dedent`
89+
<% if @condition %>
90+
<div class="a">
91+
<% else %>
92+
<div class="b">
93+
<% end %>
94+
Content
95+
</div>
96+
`
97+
98+
expectError(errorMessage("div"))
99+
assertOffenses(html)
100+
})
101+
102+
it("reports conditional open tag with button", () => {
103+
const html = dedent`
104+
<% if @with_icon %>
105+
<button class="btn btn-icon" aria-label="Action">
106+
<% else %>
107+
<button class="btn">
108+
<% end %>
109+
Click me
110+
</button>
111+
`
112+
113+
expectError(errorMessage("button"))
114+
assertOffenses(html)
115+
})
116+
117+
it("reports conditional open tag with if/elsif/else", () => {
118+
const html = dedent`
119+
<% if @style == "a" %>
120+
<div class="a">
121+
<% elsif @style == "b" %>
122+
<div class="b">
123+
<% else %>
124+
<div class="c">
125+
<% end %>
126+
Content
127+
</div>
128+
`
129+
130+
expectError(errorMessage("div"))
131+
assertOffenses(html)
132+
})
133+
134+
it("reports conditional open tag with unless", () => {
135+
const html = dedent`
136+
<% unless @plain %>
137+
<div class="fancy">
138+
<% else %>
139+
<div class="plain">
140+
<% end %>
141+
Content
142+
</div>
143+
`
144+
145+
expectError(errorMessage("div"))
146+
assertOffenses(html)
147+
})
148+
149+
it("reports multiple conditional open tags", () => {
150+
const html = dedent`
151+
<% if @first %>
152+
<div class="a">
153+
<% else %>
154+
<div class="b">
155+
<% end %>
156+
Content
157+
</div>
158+
<% if @second %>
159+
<span class="x">
160+
<% else %>
161+
<span class="y">
162+
<% end %>
163+
More content
164+
</span>
165+
`
166+
167+
expectError(errorMessage("div"))
168+
expectError(errorMessage("span"))
169+
assertOffenses(html)
170+
})
171+
})

0 commit comments

Comments
 (0)