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

[New] no-unused-modules: Support destructuring #1997

Merged

Conversation

s-h-a-d-o-w
Copy link
Contributor

Currently, when using a destructuring assignment with export, errors like this are thrown:

error  exported declaration 'undefined' not used within other modules  import/no-unused-modules

This PR fixes that issue.
Since the reasoning here seemed quite straightforward to me, I figured I'd jump straight to a PR. :)

(Minor question out of sheer curiosity: In tests, why the code duplication with code-to-be-linted in files but then also as strings within the tests?)

@coveralls
Copy link

coveralls commented Feb 28, 2021

Coverage Status

Coverage increased (+13.03%) to 84.05% when pulling ab0181d on s-h-a-d-o-w:no-unused-modules_destructuring into d31d615 on benmosher:master.

@s-h-a-d-o-w s-h-a-d-o-w force-pushed the no-unused-modules_destructuring branch from 04bc1b9 to 1f3fb2e Compare February 28, 2021 16:15
@s-h-a-d-o-w s-h-a-d-o-w force-pushed the no-unused-modules_destructuring branch from 1f3fb2e to c73b4ce Compare March 15, 2021 10:32
@s-h-a-d-o-w
Copy link
Contributor Author

s-h-a-d-o-w commented Mar 15, 2021

Whoops, hadn't realized that my fork was horribly out of date. 🤦
Sorry about that!

I don't think that I can do anything about that failed appveyor build though. Doesn't seem related to my contribution?

@ljharb
Copy link
Member

ljharb commented May 12, 2021

Does this perhaps close #918?

@ljharb ljharb force-pushed the no-unused-modules_destructuring branch from c73b4ce to a6a85cc Compare May 12, 2021 05:26
@ljharb ljharb changed the title [Fix] no-unused-modules: Support destructuring [New] no-unused-modules: Support destructuring May 12, 2021
@s-h-a-d-o-w
Copy link
Contributor Author

Nope, but it looks like that issue already got resolved along the way at some point because I wasn't able to reproduce that error just now.

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.

3 participants