Skip to content
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

Merged
merged 6 commits into from Apr 7, 2019

Conversation

Projects
None yet
3 participants
@lydell
Copy link
Contributor

lydell commented Mar 30, 2019

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).

@lydell lydell force-pushed the lydell:no-duplicates-autofix branch from 8236e24 to 9ac41f3 Mar 30, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 30, 2019

Coverage Status

Coverage increased (+3.2%) to 97.383% when pulling 9ac41f3 on lydell:no-duplicates-autofix into af976b9 on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 30, 2019

Coverage Status

Coverage increased (+3.3%) to 97.461% when pulling cca688d on lydell:no-duplicates-autofix into af976b9 on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 30, 2019

Coverage Status

Coverage increased (+3.2%) to 97.383% when pulling 9ac41f3 on lydell:no-duplicates-autofix into af976b9 on benmosher:master.

@lydell

This comment has been minimized.

Copy link
Contributor Author

lydell commented Mar 30, 2019

The ESLint v3 and v2 tests are failing. Guidance on how to solve that would be appreciated.

@lydell

This comment has been minimized.

Copy link
Contributor Author

lydell commented Mar 31, 2019

I disabled autofix for ESLint <= 3.

Show resolved Hide resolved src/rules/no-duplicates.js
Show resolved Hide resolved src/rules/no-duplicates.js Outdated
[first, ...rest].map(getDefaultImportName).filter(Boolean)
)

// Bail if there are multiple different default import names – it's up to the

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 31, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@lydell

lydell Mar 31, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@lydell

lydell Mar 31, 2019

Author Contributor

(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.)

return undefined
}

return function* (fixer) {

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 31, 2019

Collaborator

does this really need to be a generator?

This comment has been minimized.

Copy link
@lydell

lydell Mar 31, 2019

Author Contributor

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).

This comment has been minimized.

Copy link
@lydell

lydell Mar 31, 2019

Author Contributor

Confirmed – the array version works, too. Personally I like the generator here, but let me know which way you want it.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 6, 2019

Collaborator

I'd prefer the array version, I find it easier to understand.

This comment has been minimized.

Copy link
@lydell

lydell Apr 6, 2019

Author Contributor

Pushed the array version!

@lydell
Copy link
Contributor Author

lydell left a comment

  • Need to handle import * as ns
  • Add more tests for comments I thought of

(Both fixed now)

@lydell

This comment has been minimized.

Copy link
Contributor Author

lydell commented Mar 31, 2019

Ready for another round of review.

@lydell

This comment has been minimized.

Copy link
Contributor Author

lydell commented Apr 6, 2019

@ljharb Friendly ping. If you have the time to review again, that'd be great!

@ljharb

ljharb approved these changes Apr 6, 2019

Copy link
Collaborator

ljharb left a comment

LGTM but i'd prefer to see the generator refactored away

@ljharb ljharb added the semver-minor label Apr 6, 2019

@ljharb ljharb merged commit 0d812ad into benmosher:master Apr 7, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 97.461%
Details

@lydell lydell deleted the lydell:no-duplicates-autofix branch Apr 7, 2019

@lydell

This comment has been minimized.

Copy link
Contributor Author

lydell commented Apr 7, 2019

Thanks! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.