Docs: clarify defaultAssignment option, fix no-unneeded-ternary examples

[CoffeeTableEspresso]

The documentation for the no-unneeded-ternary rule seems to have a mistake. (#10872).

At one point, it mentions that a || should be used when the middle operand of the ternary is the same as the first:

// Bad
var foo = bar ? bar : 1;

// Good
var foo = bar || 1;

Later, however, it lists something seemingly identical to this as an example of correct code:

var a = x ? x : 1;

In my project, I was given a no-unneeded-ternary error for using this type of code, so the documentation seems to have an error. Either way, the documentation seems to list this sort of construct as both correct and incorrect, so one of those is probably in error.

Fixes #10872.

[jsf-clabot]

All committers have signed the CLA.

[platinumazure]

Hi @CoffeeTableEspresso, thanks for the PR.

The difference is whether the assignment is a self-assignment/-update or not. The defaultAssignment option covers self-assignments, and by default self-assignments can use ternaries.

/* eslint no-unneeded-ternary: "error" */

/**** Not a self-assignment: ****/

// Bad
var foo = bar ? bar : 1;

// Good
var foo = bar || 1;

/**** Self-assignment: ****/

// Good
var a = x ? x : 1;
// This will be allowed by default, or if the { defaultAssignment: true } option is explicitly specified

If defaultAssignment is explicitly disabled:

/* eslint no-unneeded-ternary: ["error", { defaultAssignment: false }] */

// Bad
var foo = bar ? bar : 1;

// Good
var foo = bar || 1;

/**** Self-assignment: ****/

// Bad
var a = x ? x : 1;
// This will not be allowed

I definitely think the documentation could be improved to make this more clear. If you would like to tweak this PR or submit a new one, we would greatly appreciate it. Thanks!

[CoffeeTableEspresso]

Ok, seems like I overlooked the defaultAssignment option, thanks for pointing that out.

Maybe I'm missing something still, but I don't see any difference between var foo = bar ? bar : 1; and var a = x ? x : 1;, besides the variable names, so I'm not why one would be allowed and one not with the defaults for no-unneeded-ternary set, as in your first example.

[platinumazure]

Yes, you're right, I had my own misreading. I agree the examples are basically the same and therefore contradictory. I think the best approach would be for the initial examples (which are meant to be illustrative of ternary operators in general and not meant to show how the rule works) to not use assignments- maybe they could be function call arguments instead. Then the later sections can explain that assignments are a special case. What do you think?

…

On Wed, Sep 19, 2018, 16:56 CoffeeTableEspresso ***@***.***> wrote: Ok, seems like I overlooked the defaultAssignment option, thanks for pointing that out. Maybe I'm missing something still, but I don't see any difference between var foo = bar ? bar : 1; and var a = x ? x : 1;, besides the variable names, so I'm not why one would be allowed and one not with the defaults for no-unneeded-ternary set, as in your first example. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10874 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWev052MRMM81HrCnUOpSYjt_2GYtgks5ucr2cgaJpZM4WxBH_> .

[CoffeeTableEspresso]

Yes, that was where my confusion came from.

Perhaps something like

// Bad
f(bar ? bar : 1);

// Good
f(bar || 1);

For the first example, and an extra comment explaining that the second example is only allowed because it in on the right on an assignment.

var a = x ? x : 1;   // Note that this is only allowed on the right of assignments by default. (See defaultAssignment option below.)

To clarify, are all ternaries allowed on the right of an assignment by default, or just a few specific uses?

[platinumazure]

Your proposal sounds great! I'll have to dive into the source code later to answer your last question. Thanks for your patience!

…

On Wed, Sep 19, 2018, 17:15 CoffeeTableEspresso ***@***.***> wrote: Yes, that was where my confusion came from. Perhaps something like // Bad f(bar ? bar : 1); // Good f(bar || 1); For the first example, and an extra comment explaining that the second example is only allowed because it in on the right on an assignment. var a = x ? x : 1; // Note that this is only allowed on the right of assignments by default. (See defaultAssignment option below.) To clarify, are all ternaries allowed on the right of an assignment by default, or just a few specific uses? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10874 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWes6dUqswsmgzz5I6xp0COT2qEY3Cks5ucsIXgaJpZM4WxBH_> .

[CoffeeTableEspresso]

Ok, I'll hold off for now then, since I think the comment should make it clear which types of ternary expressions are allowed in assignments. If you link the relevant source-code I could also have a look (but I'm not sure exactly how eslint is laid out so I don't know where to start looking).

[platinumazure]

Thanks! If you go to the docs page on the website, there should be a "Rule source" link at the bottom which will link to the source code directly. Otherwise I'll take a look later tonight. Thanks for the great dialogue!

…

On Wed, Sep 19, 2018, 17:24 CoffeeTableEspresso ***@***.***> wrote: Ok, I'll hold off for now then, since I think the comment should make it clear *which* types of ternary expressions are allowed in assignments. If you link the relevant source-code I could also have a look (but I'm not sure exactly how eslint is laid out so I don't know where to start looking). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10874 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWejjvsDkFgTtuYzknqV1QdtlRuLHVks5ucsQvgaJpZM4WxBH_> .

[platinumazure]

Hi again @CoffeeTableEspresso, I've dived into the rule source.

A "default assignment" is defined in the rule as follows: The ConditionalExpression test node is an Identifier, the consequent node is an Identifier, and the identifiers have the same name. So, in other words, x ? x : y (for any identifier x and any expression y). Hope this helps!

[CoffeeTableEspresso]

Ok, I have updated the file to reflect this and the changes discussed yesterday. Thanks for looking at the source code for me!

Lemme know if there's any more issues with my PR.

[platinumazure]

LGTM, I think this has come together nicely. Thanks for contributing!

I'm going to leave this open for another day or so in case any other team member would like to review.

[CoffeeTableEspresso]

Sure, no rush. Thanks for taking the time to look into this, it's been very pleasant and easy contributing to eslint.

[platinumazure]

@CoffeeTableEspresso Regarding the "commit-message" status check: Could you please update the PR title to start with "Docs:" and to overall be less than 72 characters? (And I encourage you to phrase it in terms of what you're accomplishing with this PR, because then you look more awesome in our changelog when we do our next release. 😄) Thanks!

[platinumazure]

LGTM (again, after title change). Thanks!

[platinumazure]

Merging. Thanks for contributing @CoffeeTableEspresso!

[Mouvedia]

related: #8211