-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
no-duplicates: Add autofix #1312
Conversation
8236e24
to
9ac41f3
Compare
The ESLint v3 and v2 tests are failing. Guidance on how to solve that would be appreciated. |
I disabled autofix for ESLint <= 3. |
[first, ...rest].map(getDefaultImportName).filter(Boolean) | ||
) | ||
|
||
// Bail if there are multiple different default import names – it's up to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way we could reduce the ones that are the same? ie, if the list was a, a, b, c
, it could be autofixed to a, b, c
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's possible to end up with a, a, b, c
, because duplicate identifiers are syntax errors: https://astexplorer.net/#/gist/bcba02f3c293bd99d3e00b3061befcfb/89b6673f21acde39324bb7063394b56834ede08e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also, note that the particular line you commented on isn't about the specifiers inside {...}
, but about default imports: import a from "a"; import b from "a"
cannot be autofixed.)
src/rules/no-duplicates.js
Outdated
return undefined | ||
} | ||
|
||
return function* (fixer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this really need to be a generator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible to make an array, const fixes = []
, and .push()
to it instead of yield
and return fixes
at the end. I can do that if you think that's better (and it works).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed – the array version works, too. Personally I like the generator here, but let me know which way you want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer the array version, I find it easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the array version!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Need to handle
import * as ns
- Add more tests for comments I thought of
(Both fixed now)
Ready for another round of review. |
@ljharb Friendly ping. If you have the time to review again, that'd be great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but i'd prefer to see the generator refactored away
Thanks! 🎉 |
This adds autofix to
no-duplicates
. The autofix merges duplicate imports into the first one. It bails if there are multiple default import names, or comments associated with the (non-first) duplicate imports (since it's unclear how the user wants to move them).