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
Merged

Conversation

lydell
Copy link
Contributor

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

@coveralls
Copy link

Coverage Status

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

@coveralls
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
Copy link

Coverage Status

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

@lydell
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
Copy link
Contributor Author

lydell commented Mar 31, 2019

I disabled autofix for ESLint <= 3.

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

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

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?

Copy link
Contributor Author

@lydell lydell Mar 31, 2019

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

Copy link
Contributor Author

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

return undefined
}

return function* (fixer) {
Copy link
Member

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?

Copy link
Contributor Author

@lydell lydell Mar 31, 2019

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed the array version!

Copy link
Contributor Author

@lydell lydell left a 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)

@lydell
Copy link
Contributor Author

lydell commented Mar 31, 2019

Ready for another round of review.

@lydell
Copy link
Contributor Author

lydell commented Apr 6, 2019

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

Copy link
Member

@ljharb ljharb left a 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

@ljharb ljharb merged commit 0d812ad into import-js:master Apr 7, 2019
@lydell lydell deleted the no-duplicates-autofix branch April 7, 2019 07:31
@lydell
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants