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

no-unneeded-ternary incorrect documentation for defaultAssignment option #12098

Comments

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 14, 2019

This is a "documentation bug".

The version of ESLint you are using.

6.1.0

The problem you want to solve.

Documentation for defaultAssignment option of the no-unneeded-ternary rule is incorrect.

defaultAssignment = true means don't report expressions such as x ? x : y.

defaultAssignment = false means report expressions such as x ? x : y.

Default is true.

The rule doesn't actually check if the expression is in an assignment context. That's by design from the start (#3232 and #3260). It simply searches for all ternary expressions where test and consequent are same identifier.

Incorrect parts of the documentation are:

  1. The incorrect example:
/*eslint no-unneeded-ternary: "error"*/

var a = x === 2 ? true : false;

var a = x ? true : false;

var a = f(x ? x : 1);

The example is wrong, var a = f(x ? x : 1); is not a warning because of the default option value.

  1. The correct example:
/*eslint no-unneeded-ternary: "error"*/

var a = x === 2 ? "Yes" : "No";

var a = x !== false;

var a = x ? "Yes" : "No";

var a = x ? y : x;

var a = x ? x : 1;  // Note that this is only allowed as it on the right hand side of an assignment; this type of ternary is disallowed everywhere else. See defaultAssignment option below for more details.

The comment is wrong.

  1. defaultAssignment section

The defaultAssignment option allows expressions of the form x ? x : expr (where x is any identifier and expr is any expression) as the right hand side of assignments (but nowhere else).

The option (when true) allows such expressions everywhere.

Your take on the correct solution to problem.

Fix the documentation.

Perhaps also consider changing the name of the option for two reasons:

  • The Assignment part is confusing.
  • When the option name doesn't have an explicit prefix, true ususally means "enforce on", rather than "allow".

Maybe allowSameConsequent or allowSameIfTrue.

Are you willing to submit a pull request to implement this change?

Yes, I would be glad to do it.

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Aug 19, 2019

I'm not against renaming the option if we do so in a backwards compatible way.

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

@mdjermanovic mdjermanovic commented Aug 31, 2019

Perhaps to make this issue be just a task to fix the documentation (if confirmed that it's incorrect) ?

I could open another issue to evaluate renaming.

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Sep 29, 2019

@mdjermanovic Now that you're a team member, if you can verify this is a bug you can feel free to mark it as accepted!

@samrae7

This comment has been minimized.

Copy link
Contributor

@samrae7 samrae7 commented Oct 11, 2019

I would like to make the doc change. I will verify at the same time

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

@mdjermanovic mdjermanovic commented Oct 11, 2019

@samrae7 Thanks, a PR to fix the documentation is welcome!

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

@mdjermanovic mdjermanovic commented Oct 11, 2019

Marking as accepted based on this Online Demo

The code is copy & paste from the 'incorrect' example in the docs. In this example, the rule doesn't report error for the third statement.

To avoid confusion, I removed the part about changing the option's name from the original post.

This is now just an issue to fix the docs to match the current behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.