Rule Proposal: multiline-ternary

[mysticatea]

From requireMultiLineTernary and disallowMultiLineTernary.

This rule will require/disallow multiline conditional expressions.

{
    "multiline-ternary": [
        "error",
        "always" or "never"
    ]
}

• "always" (default) - Requires a line break for each operator (? and :).
• "never" - Disallow line breaks in a conditional expression.

[kaicataldo]

I can champion this and write the rule when we get the details ironed out.

[BYK]

Isn't this already covered by operator-linebreak? (http://eslint.org/docs/rules/operator-linebreak)

[mysticatea]

No.

operator-linebreak is the rule to choose the location of operators among the head of lines or the last of lines. It doesn't require/disallow line breaks.
On the other hand, multiline-ternary requires/disallows line breaks. It allows any position of operators.

Honestly, I'm not sure this is proper for core.

[kaicataldo]

@mysticatea Curious about why you think this might not belong in core. Seems like it's general enough to me - would love to hear your thoughts

[mysticatea]

Merely I'm not sure the purpose of this rule.
So I might change my position by clear purposes.

When I'm using ternary expressions, there are 3 kind situations.

// shorter.
let foo = (a > 0 ? b : c);

// longer.
let foo = (
    this_is_a_long_expression > 0 ? 
        this_is_a_long_expression2 : 
        this_is_a_long_expression3
);

// multiple
let foo = (
    a === A ? 1 :
    a === B ? 2 :
    a === C ? 3 :
    a === D ? 4 :
    /* else */ 0
);

If I apply this rule to that code:

// never
let foo = (a > 0 ? b : c);
let foo = (this_is_a_long_expression > 0 ? this_is_a_long_expression2 : this_is_a_long_expression3);
let foo = (a === A ? 1 : a === B ? 2 : a === C ? 3 : a === D ? 4 : 0);

// always
let foo = (
    a > 0 ? 
        b : 
        c
);
let foo = (
    this_is_a_long_expression > 0 ? 
        this_is_a_long_expression2 : 
        this_is_a_long_expression3
);
let foo = (
    a === A ? 
        1 :
        a === B ? 
            2 :
            a === C ? 
                3 :
                a === D ? 
                    4 :
                    0
);

Well, the "multiple" case may be not common.

[kaicataldo]

I see - thanks for explaining. I personally don't think I would use the always or never options, but I wonder if an additional option that would enforce multiline ternaries when one or more of the expressions is longer than a configurable length might be useful.

{
    "multiline-ternary": [
        "error",
        "always" or "never" or "when-needed", { maxExpressionLength: 40 }
    ]
}

Example:

// when-needed, { maxExpressionLength: 12 }
let foo = (a > 0 ? b : c);


let foo = (
    this_is_a_long_expression > 0 ? 
        a : 
        this_is_a_long_expression3
);

"when-needed" isn't the right name, but hopefully it gets my point across. This would allow for parity with the JSCS rule while giving users a little more control. I generally follow the "when-needed" pattern when I'm writing my own code - if the expressions are longer I break it into multiple lines, but otherwise like to keep it on one.

[mysticatea]

I got it.
The third option sounds good to me. 😄

I'd like to use the end column of a ternary expression (after line breaks are removed) rather than the length of a ternary expression, but I'm not sure it's safe since it's changeable in multipass autofix.

[kaicataldo]

@mysticatea Not sure I'm following - sorry - could you show me an example of what you mean? I'm happy to get started on this when we decide what this should look like and it gets accepted :)

[mysticatea]

I'm sorry. This is an example.

/*eslint multiline-ternary: [error, {minColumn: 50}]*/

//        1         2         3         4         5
// 345678901234567890123456789012345678901234567890
let foo = condition > 0 ? value1 : value2;  // GOOD, column is 40.

function foo(cond) {
    if (cond) {
        for (let i = 0; i < 10; ++i) {
//        1         2         3         4         5         6
// 3456789012345678901234567890123456789012345678901234567890
            let longNameVariable = (a > 0 ? value1 : value2);  // BAD, column is 58.
        }
    }
}

[kaicataldo]

Thanks - I think I understand! I think you're right, that makes more sense than testing the individual expressions - allows you to configure it to the same value as max-len, for instance

[mysticatea]

Yes, max-len was in my head.
But I'm not sure whether a use of the column is safe or not.
I think difficult that interaction between rules for line breaks in multipass autofix...

[kaicataldo]

Maybe we could check for the linebreak type and make sure we only count it as one character?

[mysticatea]

Maybe we need the configuration of spacing rules in order to count column/length... :(
In interactions between other rules of line breaks or spacing, this might repeat inserting/removing line breaks...

//        1         2         3         4         5
// 345678901234567890123456789012345678901234567890
let longNameVariable = (a > 0 ? value1 : value2); // This is 46
let longNameVariable = (a > 0 ?                   // This is 46?
    value1 : 
    value2);
let longNameVariable = (a > 0 ?                   // This is 46?
value1 : 
value2);
let longNameVariable = (a>0?value1:value2); // This is...

// In a case that there are line breaks in expression...
let longNameVariable = (a > 0 ? 
value1 + // should not this line break be removed by this rule?
    "foo" +
    "bar" : 
value2);
let longNameVariable = (a > 0 ? value1 +
    "foo" +
    "bar" : value2);

// In a nested case...
let longNameVariable = (a > 0 ? value1 : b > 0 ? value2 : value3);
let longNameVariable = (a > 0 ? 
    value1 : 
    b > 0 ? 
        value2 : 
        value3);
let longNameVariable = (a > 0 ? 
    value1 : 
    b > 0 ? value2 : value3);

[kaicataldo]

Would it be worth it to make this rule now as we've talked about and just not do auto-fixing yet? Sounds like we need to figure out autofixing with newlines across the project, not just here.

Some examples, off the top of my head:

• Add auto-fixing to padded-blocks #6320
• Add auto-fixing to newline-before-return #5958

[mysticatea]

If we use length/column in an option, that info is needed to distinguish whether line breaks are needed or not.

//        1         2         3         4         5
// 345678901234567890123456789012345678901234567890
let longNameVariable = (a > 0 ?                   // Is this over 45 or not?
    value1 : 
    value2);
let longNameVariable = (a > 0 ? value1 : value2); // This is 46
let longNameVariable = (a > 0 ?value1 :value2);   // This is 44

Ah...
If this rule ignores extra line breaks, that info isn't needed.

[kaicataldo]

Hm, I see. Any ideas @eslint/eslint-team? I can get started on this as soon as we hammer the details out :)

[alberto]

I think we should leave aside the discussion about length based multiline.

Regarding the original rule proposal, never would conflict with max-len for long lines.

[kaicataldo]

@alberto why do you think we should not include a line length-based option? Curious since in my mind this allows us to have feature parity with the JSCS rules while having an option that allows for parity with max-len.

@mysticatea I think it makes sense for the option to only warn if the entire ternary is on one line and the length is too long. It could be calculated similarly to max-len then. If it's already a multi-line ternary then this option will ignore it and max-len should catch the individual lines. I like the simplicity of it.

[mikesherov]

I think we can drastically simplify this discussion by only implementing the "always" portion of this rule.

If we're concerned about "never" conflicting with "max-len", consider that JSCS likely only implemented that option because in general we always provided both rules for consistency sake (likely a mistake there). Either way, we can provide the rule, knowing it can conflict with max-len and allow users to use inline comments to address, and deprecate if we find no-one is using it?

[kaicataldo]

@mikesherov The insight into why JSCS had both is really helpful - if that's the case, I'm totally with you on just implementing "always".

I'm realizing the length-based option I outlined in my last comment has morphed into really being the same thing as turning off this proposed rule and turning on max-len, so I'm dropping my support of a third option.

TL;DR I'm 👍 for @mikesherov's suggestion

[alberto]

@kaicataldo because the priority is JSCS compatibility and there is some uncertainty in the value that option provides when max-len already exists and wether it can be done since it would need to know the tab width.

@mikesherov since we decided we don't want to deprecate options in existing rules, I think it would be preferable to only add always now and discuss about the never use-case if someone asks for it.

[kaicataldo]

@alberto thanks for the explanation - I'm dropping my support for the third option.

Sounds like we have three team members in agreement about implementing this rule with only the "always" option (or I guess without any configurable options, at this point!).

Thoughts, @nzakas?

[nzakas]

Seems reasonable to me 👍.

[butterflyhug]

I don't currently need tooling to enforce this, so feel free not to implement it -- but my personal style uses never here, and the clash with max-len would be exactly what I'd be trying to enforce. I generally don't write something as a ternary if the ternary won't fit on a single normal-length line. I think the vast majority of those longer ternaries are more readable (and still similarly concise in terms of line count) when converted to if or switch.

[kaicataldo]

Starting working on this and realized I have some questions:

1. How do we feel about autofixing for this? I can arbitrarily choose to insert a newline before or after the ? and :, but since operator-linebreak does not have an autofix, autofixing could potentially just create another issue the user needs to manually fix.

2. Should I be writing this with Node 4-compatible ES6 features? It won't actually pass the test task with the current conf/eslint.json configuration - would we want to modify that? Or does that change involve more than that? I'll look into it more myself, but figured it might quicker to ask someone more knowledgeable. I could also not use any ES6 features and we could update this as we go through and update the rest of the codebase as part of this issue.

[platinumazure]

Just my two cents- I think auto-fix on this rule will create more problems than it solves.

One other thought: Does JSCS attempt to fix this?

[kaicataldo]

@platinumazure I agree with you, and no, I don't think the JSCS rule fixes this