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

[karthikkp]

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.

[platinumazure]

Thanks for contributing! Left a few suggestions.

[platinumazure]

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]

@platinumazure Done.

[mysticatea]

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]

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

[platinumazure]

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]

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

[mysticatea]

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]

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

[mysticatea]

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

[karthikkp]

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

[mysticatea]

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?

[mysticatea]

LGTM.

Thank you very much!

[kaicataldo]

LGTM. Thanks for contributing!

[kaicataldo]

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

[karthikkp]

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

[aladdin-add]

merged, thanks for contributing! ❤️