Skip to content

Commit 5bebad3

Browse files
mskeltonmskelton
committed
fix: Support comments with sort group separators
1 parent 4b26875 commit 5bebad3

File tree

4 files changed

+140
-33
lines changed

4 files changed

+140
-33
lines changed

‎docs/rules/imports.md

+2
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,8 @@ newlines between sort groups.
115115
{
116116
"groups": [
117117
{ "type": "side-effect", "order": 1 },
118+
{ "regex": "\\.(png|jpg|svg)$", "order": 4 },
119+
{ "type": "dependency", "order": 2 },
118120
{ "type": "other", "order": 3 }
119121
],
120122
"separator": "\n"

‎src/__tests__/imports.spec.ts

+114-2
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,7 @@ ruleTester.run("sort/imports", rule, {
373373
errors: [
374374
{
375375
messageId: "missingSeparator",
376-
data: { separator: "\\n" },
376+
data: { expected: "\\n" },
377377
line: 3,
378378
column: 1,
379379
endLine: 3,
@@ -493,7 +493,7 @@ ruleTester.run("sort/imports", rule, {
493493
},
494494
{
495495
messageId: "missingSeparator",
496-
data: { separator: "\\n" },
496+
data: { expected: "\\n" },
497497
line: 8,
498498
column: 1,
499499
endLine: 8,
@@ -579,5 +579,117 @@ ruleTester.run("sort/imports", rule, {
579579
},
580580
],
581581
},
582+
583+
// Separators + comments
584+
{
585+
name: "Adds separators when there are comments",
586+
code: dedent`
587+
// a
588+
import y from "y.png"
589+
// b
590+
import z from "z.png"
591+
// c
592+
import c from "c"
593+
// d
594+
import d from "d"
595+
`,
596+
// This test doesn't have the correct output due to conflicting ranges,
597+
// but we can at least test that the right errors are present.
598+
output: dedent`
599+
// a
600+
import y from "y.png"
601+
// b
602+
import z from "z.png"
603+
604+
// c
605+
import c from "c"
606+
// d
607+
import d from "d"
608+
`,
609+
errors: [
610+
{
611+
messageId: "missingSeparator",
612+
data: { expected: "\\n" },
613+
line: 5,
614+
column: 1,
615+
endLine: 5,
616+
endColumn: 1,
617+
},
618+
],
619+
options: [
620+
{
621+
groups: [
622+
{ regex: "\\.(png|jpg)$", order: 1 },
623+
{ type: "other", order: 2 },
624+
],
625+
separator: "\n",
626+
},
627+
],
628+
},
629+
{
630+
name: "Removes extra newlines while preserving comment position",
631+
code: dedent`
632+
// a
633+
import y from "y.png"
634+
635+
// b
636+
import z from "z.png"
637+
638+
639+
// c
640+
import c from "c"
641+
642+
// d
643+
import d from "d"
644+
`,
645+
// This test doesn't have the correct output due to conflicting ranges,
646+
// but we can at least test that the right errors are present.
647+
output: dedent`
648+
// a
649+
import y from "y.png"
650+
// b
651+
import z from "z.png"
652+
653+
// c
654+
import c from "c"
655+
// d
656+
import d from "d"
657+
`,
658+
errors: [
659+
{
660+
messageId: "extraNewlines",
661+
data: { newlines: "newline" },
662+
line: 3,
663+
column: 1,
664+
endLine: 3,
665+
endColumn: 1,
666+
},
667+
{
668+
messageId: "incorrectSeparator",
669+
data: { expected: "\\n", actual: "\\n\\n" },
670+
line: 6,
671+
column: 1,
672+
endLine: 7,
673+
endColumn: 1,
674+
},
675+
{
676+
messageId: "extraNewlines",
677+
data: { newlines: "newline" },
678+
line: 10,
679+
column: 1,
680+
endLine: 10,
681+
endColumn: 1,
682+
},
683+
],
684+
options: [
685+
{
686+
groups: [
687+
{ regex: "\\.(png|jpg)$", order: 1 },
688+
{ type: "other", order: 2 },
689+
],
690+
separator: "\n",
691+
},
692+
],
693+
},
582694
],
583695
})

‎src/__tests__/type-properties.spec.ts

+6-8
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,12 @@ const createValidCodeVariants = (
99
): TSESLint.RunTests<
1010
"unsorted",
1111
[{ caseSensitive?: boolean; natural?: boolean }]
12-
>["valid"] => {
13-
return [
14-
{ code, options: [{ caseSensitive: false, natural: false }] },
15-
{ code, options: [{ caseSensitive: true, natural: false }] },
16-
{ code, options: [{ caseSensitive: false, natural: true }] },
17-
{ code, options: [{ caseSensitive: true, natural: true }] },
18-
]
19-
}
12+
>["valid"] => [
13+
{ code, options: [{ caseSensitive: false, natural: false }] },
14+
{ code, options: [{ caseSensitive: true, natural: false }] },
15+
{ code, options: [{ caseSensitive: false, natural: true }] },
16+
{ code, options: [{ caseSensitive: true, natural: true }] },
17+
]
2018

2119
ruleTester.run("sort/type-properties", rule, {
2220
valid: [

‎src/rules/imports.ts

+18-23
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,17 @@ export default {
104104
for (let i = 1; i < nodes.length; i++) {
105105
const node = nodes[i]
106106
const prevNode = nodes[i - 1]
107+
108+
// If the node has preceding comments, we want to use the first
109+
// comment as the starting position for both determining what the
110+
// current separator is as well as the location for the report.
111+
const nodeOrComment = source.getCommentsBefore(node)[0] ?? node
112+
113+
// Find the range between nodes (including comments) so we can pull
114+
// the text and apply fixes to newlines between imports.
107115
const rangeBetween: AST.Range = [
108116
range.end(prevNode),
109-
range.start(node),
117+
range.start(nodeOrComment),
110118
]
111119

112120
// To make the separator option make more sense, we always assume a
@@ -117,7 +125,6 @@ export default {
117125
// stable even if there is extra spaces before imports.
118126
const actualSeparator = text
119127
.slice(...rangeBetween)
120-
.replace(/\n[^\n]+/g, "") // Remove lines with content (e.g. comments)
121128
.replace(/[^\n]/g, "") // Remove all non-newline characters
122129
.replace("\n", "") // Remove the first newline
123130

@@ -127,15 +134,10 @@ export default {
127134
// message to the second import since it makes more sense that it
128135
// wasn't spaced far enough from the first one (reading order top down).
129136
const startLine = (prevNode.loc?.end.line ?? 0) + 1
137+
const endLine = (nodeOrComment.loc?.start.line ?? 0) - 1
130138
const loc: AST.SourceLocation = {
131-
start: {
132-
line: startLine,
133-
column: 0,
134-
},
135-
end: {
136-
line: Math.max((node.loc?.start.line ?? 0) - 1, startLine),
137-
column: 0,
138-
},
139+
start: { line: startLine, column: 0 },
140+
end: { line: Math.max(endLine, startLine), column: 0 },
139141
}
140142

141143
const isSameGroup =
@@ -152,21 +154,15 @@ export default {
152154
data: {
153155
newlines: pluralize("newline", actualSeparator.length),
154156
},
155-
fix(fixer) {
156-
return fixer.replaceTextRange(rangeBetween, "\n")
157-
},
157+
fix: (fixer) => fixer.replaceTextRange(rangeBetween, "\n"),
158158
})
159159
}
160160
} else if (separator !== "" && actualSeparator === "") {
161161
context.report({
162162
messageId: "missingSeparator",
163163
loc,
164-
data: {
165-
separator: rawString(separator),
166-
},
167-
fix(fixer) {
168-
return fixer.insertTextBefore(node, separator)
169-
},
164+
data: { expected: rawString(separator) },
165+
fix: (fixer) => fixer.insertTextAfter(prevNode, separator),
170166
})
171167
} else if (separator !== actualSeparator) {
172168
context.report({
@@ -176,9 +172,8 @@ export default {
176172
actual: rawString(actualSeparator),
177173
expected: rawString(separator),
178174
},
179-
fix(fixer) {
180-
return fixer.replaceTextRange(rangeBetween, separator + "\n")
181-
},
175+
fix: (fixer) =>
176+
fixer.replaceTextRange(rangeBetween, separator + "\n"),
182177
})
183178
}
184179
}
@@ -195,7 +190,7 @@ export default {
195190
incorrectSeparator:
196191
"Expected `{{expected}}` to separate import groups but found `{{actual}}`.",
197192
extraNewlines: "Unexpected {{newlines}} between imports.",
198-
missingSeparator: "Missing `{{separator}}` between import groups.",
193+
missingSeparator: "Missing `{{expected}}` between import groups.",
199194
unsorted: "Imports should be sorted.",
200195
},
201196
schema: [

0 commit comments

Comments
 (0)