-
Notifications
You must be signed in to change notification settings - Fork 160
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
fix: no-console rule update #391
Conversation
(node.expression.expression.name === 'console' || | ||
node.expression.expression.name === 'console2') && | ||
node.expression.memberName.startsWith('log') | ||
) | ||
} | ||
|
||
isAnImport(node) { | ||
return ( | ||
node.type === 'ImportDirective' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this in the previous review, but this check doesn't make sense anymore. Since you are using the ImportDirective
callback, this check will always be true.
I think I would just inline this logic in the ImportDirective
callback, this indirection is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't understand what you mean.
As I understood:
ImportDirective is executed if/when the code has an import... so that is calling the isAnImport() function to check the import path
FunctionCall is executed if/when a call to a function is being done... like console.log, so that is what the function isConsoleLog is checking with that expression
I did delete line 42 which was useless
Please post your suggestion or approve the pr if you consider we can move past this. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ImportDirective
callback is called when an import node is visited, so checking if it's of type 'ImportDirective' is redundant. I pushed a commit fixing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhh sorry
I thought you were talking about these checks
node.expression.expression.name === 'console' ||
node.expression.expression.name === 'console2') &&
node.expression.memberName.startsWith('log')
that's why did not make sense for me...
Yes, it was useless to check if that was an importDirective since it is being called by that node
f64a927
to
0ced110
Compare
There was an error in the no-console rule preventing to invoke any method in a variable including the 'console' string.
This update fix that. Tests included.