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

Update: rule no-console should ignore assignment. #8961

Closed
wants to merge 7 commits into from

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Jul 18, 2017

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?

@eslintbot
Copy link

LGTM

@platinumazure
Copy link
Member

Are you sure you have the correct issue number?

@aladdin-add
Copy link
Member Author

@platinumazure corrected.

Copy link
Member

@platinumazure platinumazure left a 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.

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

Copy link
Member

@platinumazure platinumazure left a 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!

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jul 18, 2017

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 console.log(), rather than adding a few special exceptions for reassignment? For example, this PR won't solve the use case from #7806 (comment).

@eslintbot
Copy link

LGTM

@aladdin-add
Copy link
Member Author

I'm 👍 to @not-an-aardvark . so I modified the implement.

if there is a consensus to change, please let me know.

@eslintbot
Copy link

LGTM

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint needs bikeshedding Minor details about this change need to be discussed rule Relates to ESLint's core rules labels Jul 23, 2017
Copy link
Member

@not-an-aardvark not-an-aardvark left a 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.

@@ -122,6 +122,9 @@ module.exports = {
if (!shadowed) {
references
.filter(isMemberAccessExceptAllowed)

// only report errors on CallExpression
.filter(item => item.identifier.parent.parent.type === "CallExpression")
Copy link
Member

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)

@not-an-aardvark
Copy link
Member

@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.

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@not-an-aardvark
Copy link
Member

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.

@not-an-aardvark not-an-aardvark added the do not merge This pull request should not be merged yet label Sep 1, 2017
@not-an-aardvark
Copy link
Member

In yesterday's TSC meeting, the TSC decided that we should update the documentation for no-console to specify how to use no-restricted-syntax to achieve the other behavior, rather than changing the behavior of the function by default.

@aladdin-add aladdin-add closed this Sep 2, 2017
@aladdin-add aladdin-add deleted the issue7086 branch October 20, 2017 18:42
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Mar 2, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion do not merge This pull request should not be merged yet enhancement This change enhances an existing feature of ESLint needs bikeshedding Minor details about this change need to be discussed rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-console rule should document how to enforce calls only via no-restricted-syntax
4 participants