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: warn about mixing ternary and logical operators (fixes #11704) #12001

Merged
merged 10 commits into from Aug 13, 2019
Merged

Update: warn about mixing ternary and logical operators (fixes #11704) #12001

merged 10 commits into from Aug 13, 2019

Conversation

karthikkp
Copy link
Contributor

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

#11704

What changes did you make? (Give an overview)
Added additional check to see if the parent is a conditional expression and see if it's mixed and added messages for the same

Is there anything you'd like reviewers to focus on?
Nothing in particular.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jul 17, 2019
@platinumazure platinumazure 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 rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jul 17, 2019
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.

Thanks for contributing! Left a few suggestions.

lib/rules/no-mixed-operators.js Outdated Show resolved Hide resolved
docs/rules/no-mixed-operators.md Outdated Show resolved Hide resolved
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.

Code/tests/docs LGTM, thanks!

Just one more thing: Could you please change the PR title to begin with "Update:" rather than "Fix:"? This is a rule enhancement, not a bug fix.

@karthikkp karthikkp changed the title Fix: warn about mixing ternary and logical operators (fixes #11704) Update: warn about mixing ternary and logical operators (fixes #11704) Jul 17, 2019
@karthikkp
Copy link
Contributor Author

@platinumazure Done.

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Hi. Thank you for your contribute!

This PR looks to handle only test. However, I think that it should handle all of the ternary parts; test, consequent, and alternate.

@mysticatea
Copy link
Member

Also, it must not change the default behavior. It needs options for ternary operators.

@platinumazure
Copy link
Member

Ah, yes, @mysticatea is right. We can't change the default behavior in a minor release. So please add the ternary check to a new option (default: off). We can then change the default to true (and/or deprecate the option) in our next major release.

Sorry for missing that, @karthikkp.

@karthikkp
Copy link
Contributor Author

karthikkp commented Jul 19, 2019

@mysticatea "var foo = a && b ? c || d && o : e || f;" will this be a right example for the case you are talking about, the alternate and consequence parts?

I am checking if there are parenthesis for the conditional expression as whole and any logical expressions before the conditional. checking within the conditional expression for mixed operators seems to be already done and this is what I got for the above input.

Screen Shot 2019-07-19 at 11 29 46 AM

@mysticatea
Copy link
Member

mysticatea commented Jul 19, 2019

First, it needs an option. For example:

{
    "no-mixed-operators": [
        "error",
        {
            "groups": [
                ["?:", "||", "&&"], // disallow mix of ternary expression and logical expression
            ],,
            "allowSamePrecedence": false
        }
    ]
}

Then the rule will disallow the following cases:

a && b ? 1 : 2 // ⇒ `(a && b) ? 1 : 2` or `a && (b ? 1 : 2)`
x ? a && b : 0 // ⇒ `x ? (a && b) : 0`
x ? 0 : a && b // ⇒ `(x ? 0 : a) && b` or `x ? 0 : (a && b)`
a ? b : c ? d : e // ⇒ `(a ? b : c) ? d : e` or `a ? b : (c ? d : e)`
a ? b ? c : d : e // ⇒ `a ? (b ? c : d) : e`

Also, it's not only ternary and logical. For example, the option ["?:", "+", "-"] will check mix of ternary expressions and add/subtract expressions.

I'd like to see test cases, such as above.

@karthikkp
Copy link
Contributor Author

@mysticatea nested ternary operators already have a rule that warns. The current changes gives warnings to the remaining cases you have mentioned.

@mysticatea
Copy link
Member

Oh, you are right. Nested ternary expressions are not the scope of this rule. I'm sorry.

@karthikkp
Copy link
Contributor Author

@mysticatea Added the option to ignore the ternary operator or check for it. Also added are few more test cases.

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Thank you for the update.

The direction looks good to me. But I have a few concerns.

  • Would you update documentation for the new option?
  • Would you add valid test cases for the new option?

tests/lib/rules/no-mixed-operators.js Outdated Show resolved Hide resolved
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM.

Thank you very much!

Copy link
Member

@kaicataldo kaicataldo 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!

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

One small comment about the new JSDoc block, but otherwise LGTM!

lib/rules/no-mixed-operators.js Outdated Show resolved Hide resolved
@karthikkp
Copy link
Contributor Author

@mysticatea How many approvals do you require to merge a PR?

@aladdin-add aladdin-add merged commit 1aff8fc into eslint:master Aug 13, 2019
@aladdin-add
Copy link
Member

merged, thanks for contributing! ❤️

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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants