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
Update: rule no-console should ignore assignment. #8961
Conversation
LGTM |
Are you sure you have the correct issue number? |
@platinumazure corrected. |
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 think we should only avoid reporting if console.log
(etc.) are on the left-hand side of an AssignmentExpression.
In other words, this should be an invalid test:
const foo = console.log;
But with this proposed implementation, I believe it will be considered valid.
LGTM |
LGTM |
LGTM |
LGTM |
LGTM |
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.
LGTM, thanks for contributing!
This will still report things like [console.log] = [0];
({ foo: console.log } = { foo: 1 });
for (console.log in { a: 1, b: 2, c: 3 }); Based on #7806 (comment), would it be better to just disallow calls to |
LGTM |
I'm 👍 to @not-an-aardvark . so I modified the implement. if there is a consensus to change, please let me know. |
LGTM |
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.
Thanks! This mostly looks good, I just found a small issue.
lib/rules/no-console.js
Outdated
@@ -122,6 +122,9 @@ module.exports = { | |||
if (!shadowed) { | |||
references | |||
.filter(isMemberAccessExceptAllowed) | |||
|
|||
// only report errors on CallExpression | |||
.filter(item => item.identifier.parent.parent.type === "CallExpression") |
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.
This will report errors when console.log
is an argument:
foo(console.log);
Based on #7806 (comment) I think that should not report an error.
This could be fixed by doing something like:
- .filter(item => item.identifier.parent.parent.type === "CallExpression")
+ .filter(item => item.identifier.parent.parent.type === "CallExpression" && item.identifier.parent === item.identifier.parent.parent.callee)
@platinumazure I added the "needs bikeshedding" label in response to #7806 (comment), but feel free to remove it and/or relabel the PR if you're no longer in favor of this at all. |
LGTM |
LGTM |
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.
LGTM, thanks!
Waiting to merge this until the discussion in #7806 (comment) is resolved. Personally I'm fine with the semantics in this PR, but we should get consensus on it before merging. |
In yesterday's TSC meeting, the TSC decided that we should update the documentation for |
What is the purpose of this pull request? (put an "X" next to item)
[x] Changes an existing rule (template)
What changes did you make? (Give an overview)
fixes #7806
Is there anything you'd like reviewers to focus on?