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

[import/no-unused-modules] Report error on export specifier node #2842

Merged
merged 2 commits into from Jul 28, 2023

Conversation

Chamion
Copy link
Contributor

@Chamion Chamion commented Jul 26, 2023

I noticed when an export declaration has multiple specifiers of which only a subset is unused the rule reports the error on the whole statement. In my code editor this draws a red squiggly under the whole statement. I can read which specifier is unused from the message of the reported error. Wouldn't it be nicer if the error was reported only on the specifier that is unused? That way I wouldn't necessarily need to even read and can let the red squiggly lead my monkey brain.

Consider the following export declaration

export { foo, bar } from './foo';

Assume foo is used, bar is unused. Usually the fix would be to remove bar. I'd expect the reported error to point to that node as it would make it slightly more intuitive to attack it directly.

I'd imagine this change isn't breaking as the same errors still get reported for the same reasons, just pointing to different nodes.

I couldn't think of a reason one would want the report on the parent declaration instead but if there is one please educate me.

I changed the test assertions that have to do with a declaration that has multiple specifiers to also assert a specific location for the reported errors. I changed the node the errors are reported on to make the tests pass again. I then reverted the test changes because CI disagreed even though they passed locally 🤷.

@ljharb
Copy link
Member

ljharb commented Jul 26, 2023

I am very surprised that test changes wouldn't be required - can we remove the revert, so i can look into the failures?

@Chamion
Copy link
Contributor Author

Chamion commented Jul 27, 2023

can we remove the revert, so i can look into the failures?

The history of the failed test runs is still available.

@Chamion
Copy link
Contributor Author

Chamion commented Jul 27, 2023

The tests failed on endLine so I knew line had passed. I tried removing just the end assertions and that seems to work.

I still don't know why it didn't work but this is almost as good in terms of test strictness.

@ljharb ljharb merged commit 90e2dfa into import-js:main Jul 28, 2023
162 of 163 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants