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

Errors for commented out imports and exports #42

Closed
alubbe opened this issue Feb 18, 2020 · 9 comments · Fixed by #43
Closed

Errors for commented out imports and exports #42

alubbe opened this issue Feb 18, 2020 · 9 comments · Fixed by #43
Labels
bug Something isn't working important Important issue

Comments

@alubbe
Copy link

alubbe commented Feb 18, 2020

The tool does not recognize when an invalid import statement has been commented out, like this:

// export { nonsense } from './invalid/path';

It throws this error until we delete the line:

UnhandledPromiseRejectionWarning: Error: Cannot find module './invalid/path' from 'some/folder'

It would be great if it would ignore these lines - does the currently logic depend on pattern matching or does it parse the AST of the files?

@sQVe
Copy link
Collaborator

sQVe commented Feb 18, 2020

@alubbe Nice find!

The current logic utilizes pattern matching to find imports and exports. It should of course ignore commented out lines.

@sQVe sQVe added bug Something isn't working important Important issue labels Feb 18, 2020
@alubbe
Copy link
Author

alubbe commented Feb 18, 2020

Okay so then ignoring lines with a preceding // is simple, but I'm not sure what the most economic/effient way would be to find out whether you're in a (possible nested) /* ... */ block - how would you resolve that with pattern matching?

@sQVe sQVe changed the title Does not ignored commented out imports Errors for commented out imports and exports Feb 18, 2020
@alubbe
Copy link
Author

alubbe commented Feb 18, 2020

Ok so JavaScript does not have nested comments, so it suffices to find the last */ and then scan if there are any /* between that and your current position

@alubbe
Copy link
Author

alubbe commented Feb 18, 2020

or, more economically, find the nearest preceding /* and the see if it has been closed by a */ before your current position - but you probably want to keep track of all of this per file because otherwise you're doing this per line with an import statement

@alubbe
Copy link
Author

alubbe commented Feb 18, 2020

anyways, thanks for this tool - I hope to try it again once it ignores comments (atm our code base is just too large and littered with comments for me to clean up by hand)

@sQVe
Copy link
Collaborator

sQVe commented Feb 18, 2020

@alubbe There are plenty of ways to solve this problem. I'll ping you once it is done. Cheers for the input. 👍

@sQVe
Copy link
Collaborator

sQVe commented Feb 18, 2020

@alubbe This should now be fixed on the next release of destiny.

@alubbe
Copy link
Author

alubbe commented Feb 18, 2020

amazing, thanks!

@sQVe
Copy link
Collaborator

sQVe commented Feb 20, 2020

@alubbe This is now released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working important Important issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants