no-duplicates: Bail autofix if first import has problematic comments

btk5h · Mar 31, 2019 · f74a74e · f74a74e
1 parent cdb7c37
commit f74a74e
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 10 deletions.
diff --git a/src/rules/no-duplicates.js b/src/rules/no-duplicates.js
@@ -36,8 +36,11 @@ function getFix(first, rest, sourceCode) {
     return undefined
   }
 
-  // If the first import is `import * as ns from './foo'` there's nothing we can do.
-  if (hasNamespace(first)) {
+  // Adjusting the first import might make it multiline, which could break
+  // `eslint-disable-next-line` comments and similar, so bail if the first
+  // import has comments. Also, if the first import is `import * as ns from
+  // './foo'` there's nothing we can do.
+  if (hasProblematicComments(first, sourceCode) || hasNamespace(first)) {
     return undefined
   }
 
@@ -51,15 +54,11 @@ function getFix(first, rest, sourceCode) {
     return undefined
   }
 
-  // It's not obvious what the user wants to do with comments associated with
-  // duplicate imports, so skip imports with comments when autofixing. Also skip
-  // `import * as ns from './foo'` imports, since they cannot be merged into
-  // another import.
+  // Leave it to the user to handle comments. Also skip `import * as ns from
+  // './foo'` imports, since they cannot be merged into another import.
   const restWithoutComments = rest.filter(node => !(
-      hasCommentBefore(node, sourceCode) ||
-      hasCommentAfter(node, sourceCode) ||
-      hasCommentInsideNonSpecifiers(node, sourceCode) ||
-      hasNamespace(node)
+    hasProblematicComments(node, sourceCode) ||
+    hasNamespace(node)
   ))
 
   const specifiers = restWithoutComments
@@ -179,6 +178,16 @@ function hasSpecifiers(node) {
   return specifiers.length > 0
 }
 
+// It's not obvious what the user wants to do with comments associated with
+// duplicate imports, so skip imports with comments when autofixing.
+function hasProblematicComments(node, sourceCode) {
+  return (
+    hasCommentBefore(node, sourceCode) ||
+    hasCommentAfter(node, sourceCode) ||
+    hasCommentInsideNonSpecifiers(node, sourceCode)
+  )
+}
+
 // Checks whether `node` has a comment (that ends) on the previous line or on
 // the same line as `node` (starts).
 function hasCommentBefore(node, sourceCode) {

diff --git a/tests/src/rules/no-duplicates.js b/tests/src/rules/no-duplicates.js
@@ -160,6 +160,21 @@ ruleTester.run('no-duplicates', rule, {
       errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
     }),
 
+    test({
+      code: `
+        // some-tool-disable-next-line
+        import {x} from './foo'
+        import {//y\ny} from './foo'
+      `,
+      // Autofix bail because of comment.
+      output: `
+        // some-tool-disable-next-line
+        import {x} from './foo'
+        import {//y\ny} from './foo'
+      `,
+      errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
+    }),
+
     test({
       code: `
         import {x} from './foo'
@@ -175,6 +190,19 @@ ruleTester.run('no-duplicates', rule, {
       errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
     }),
 
+    test({
+      code: `
+        import {x} from './foo' // some-tool-disable-line
+        import {y} from './foo'
+      `,
+      // Autofix bail because of comment.
+      output: `
+        import {x} from './foo' // some-tool-disable-line
+        import {y} from './foo'
+      `,
+      errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
+    }),
+
     test({
       code: `
         import {x} from './foo'
@@ -299,5 +327,22 @@ ruleTester.run('no-duplicates', rule, {
       `,
       errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
     }),
+
+    test({
+      code: `
+        import {x} from
+        // some-tool-disable-next-line
+        './foo'
+        import {y} from './foo'
+      `,
+      // Autofix bail because of comment.
+      output: `
+        import {x} from
+        // some-tool-disable-next-line
+        './foo'
+        import {y} from './foo'
+      `,
+      errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
+    }),
   ],
 })