Add prefer-ternary rule

[jmoore914]

Fixes #486

[fisker]

I think we can also add new , throw, await, yeild as well.

[sindresorhus]

This should not be a ternary even with an option.

[fisker]

Better example should be

if (bar) {
	throw new Error('foo');
} else{
	throw new Error('bar');
}

[jmoore914]

Sorry, I'm not fully understanding. Are the two of you disagreeing that this should be a use case or are you agreeing that it is just a bad example?

[sindresorhus]

I'm saying that throw should never be made into a ternary. @fisker is commenting about the example itself as you're throwing a number, which is not valid, and the code style is off.

[sindresorhus]

Same as https://github.com/sindresorhus/eslint-plugin-unicorn/pull/514/files#r380260375. It's a bad practice to put void function calls in a ternary. Only function calls that return a value should be in a ternary.

[sindresorhus]

Basically; If all the conditional branches assign to the same thing, it can be made a ternary, if not, we don't do anything. No options needed.

[jmoore914]

Basically; If all the conditional branches assign to the same thing, it can be made a ternary, if not, we don't do anything. No options needed.

Yep, that's what my thinking was as well. I would add return as well though or do you disagree? You could say that return would be assigning to the same value which is the function itself.

[sindresorhus]

If all the branches return, then yes, that would be possible too.

[fisker]

As I understand, we can check the two branches has same node type.

Same AssignmentExpression: (only check left is the same)

if (foo) {
	bar = A;
} else {
	bar = B;
}

Same ReturnStatement:

if (foo) {
	return A;
} else {
	return B;
}

Same ThrowStatement:

if (foo) {
	throw A;
} else {
	throw B;
}

and same YieldExpression or same AwaitExpression

---

No need care what A B is, unless they are already a ternary. Even

if (foo) {
	return function() {};
} else {
	return a.b.c.d.e.f();
}

If you want, you can compare deeper and try merge the same part of A and B, for example:

if (foo) {
	return a(1, 2, 3, 4);
} else {
	return a(1, 2, 3, 5);
}

You can simply fix to return foo ? a(1, 2, 3, 4) : a(1, 2, 3, 5) or you can fix to return a(1, 2, 3, foo ? 4 : 5), if you want compare this two nodes. I think both acceptable.

[fisker]

Another thing, I suggest alway add () to the condition, no-extra-parens rule can remove it if it's not necessary, this can avoid strange stuff

if (a = foo()) {return A} {return B} 
// return a = foo() ? A : B
// return (a = foo()) ? A : B

[jmoore914]

@fisker Can you give me an example of an Await statement that would trigger the rule? I am not seeing how it is any different from a normal function call which we do not want included in the rule.

[fisker]

if (foo) {
	await promise1;
} else {
	await promise2;
}

can fix to

await (foo ? promise1 : promise2)

[jmoore914]

I'm not sure whether I would agree that a ternary is preferable in that case. @sindresorhus , do you have any thoughts regarding await?

[sindresorhus]

Ideally, it should be:

const something = foo ? promise1 : promise2;
await something;

But I think await (foo ? promise1 : promise2) is fine too.

[fisker]

This part is not good, you can compare type first, something like

if (isSameType(node)) {
  // Handle `AssignmentExpression`
  // ...
  // Handle `ReturnStatement`
  // ...
  // Handle `ThrowStatement`
  // ...
  // Handle `ExpressionStatement > AwaitExpression`
  // ...
  // ...
}

[fisker]

We have a trouble here, the catch-error-name and prevent-abbreviations fix err to error, and this rule is creating const error = test ? a : b; cause Parsing error: Identifier 'error' has already been declared.

I have no idea how to fix this.

[fisker]

Actual code https://github.com/eslint/eslint/blob/b76aef778befb32afe7ad249934b132dc49713d2/lib/shared/config-validator.js#L128

[fisker]

@futpib

Glad to see you here, you are the one add avoidCapture(great util, BTW), do you have an idea how to fix this problem?

I was thinking use a global Map to manage the variables created by our rules, but I'm not sure if it will work. And this may still can't solve the real problem, if there is a new variable created by other rule from another plugin.

[futpib]

Even if we make all rules in eslint-plugin-unicorn generate non-conflicting names, we can still hit name conflicts across eslint plugins, so I think this is a eslint issue.

Having a plugin-global map seems like an ok workaround. (It should probably be a WeakMap<Scope, Set<string>> like in prevent-abbreviations)

eslint-plugin-unicorn/rules/prevent-abbreviations.js

Lines 552 to 558 in 16f6ef3

	// Holds a map from a `Scope` to a `Set` of new variable names generated by our fixer.
	// Used to avoid generating duplicate names, see for instance `let errCb, errorCb` test.
	const scopeToNamesGeneratedByFixer = new WeakMap();
	const isSafeName = (name, scopes) => scopes.every(scope => {
	const generatedNames = scopeToNamesGeneratedByFixer.get(scope);
	return !generatedNames || !generatedNames.has(name);
	});

EDIT: A rule-global map is ok for this PR. Making it plugin-global is an issue for another PR to fix.

[sindresorhus]

we can still hit name conflicts across eslint plugins, so I think this is a eslint issue.

Can you open an issue on https://github.com/eslint/eslint ?

[fisker]

eslint/eslint#13706

[fisker]

A note to myself, the if block can be some node's body, which will break by current fix, need investigate.

Example:

while (foo) if (test) {throw a} else {throw b}

Wrong:

while (foo) const error = test ? a : b;
throw error;

Correct:

while (foo) {
  const error = test ? a : b;
  throw error;
}

[sindresorhus]

Bump

[sindresorhus]

@jmoore914 Bump

[fisker]

Fixed #514 (comment) with this suggestion.

#514 (comment) also fixed.

Ready for review.

[fisker]

Result on prettier codebase fisker/prettier#807