Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add prefer-conditional-expression rule #2363

Merged
merged 8 commits into from May 15, 2017

Conversation

andy-hanson
Copy link
Contributor

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

Added the prefer-conditional rule, which warns on let x; if (so) { x = 1; } else { x = 2; }.
Not sure if this should do anything about if (so) { return 1; } else { return 2; }. Thoughts?

CHANGELOG.md entry:

[new-rule] prefer-conditional

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a better name would be prefer-ternary-declaration


if (true)
~~ [X]
x = "TRUE";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if there is complex logic in these branches? I would prefer to use this more verbose pattern over a ternary if switching to a ternary meant a very long multiline statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule needs x = <<code>> exactly, it doesn't just search to see if it appears somewhere. So normally that would filter out complex logic.
Maybe you were thinking of something like this?

let x: number[];
if (so) {
	x = [1,2,3].map(n => {
		switch (n) {
			... lots of code ...
		}
	});
} else {
	x = [];
}

Maybe the rule could only activate if the expression takes up a single line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a good example. In that case I personally would prefer to keep the if / else blocks; I generally only want to use ternarys when each RHS expression is a single line. If this is going to be a core rule, I think it should be more conservative than it is now -- can you make that change?

@@ -117,6 +117,7 @@ export const rules = {
"no-use-before-declare": true,
"no-var-keyword": true,
"no-void-expression": true,
"prefer-conditional": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please change the rule name? I think simply "conditional" is too vague because if-else statements are often described as conditionals as well. For example, no-conditional-assignment applies to the conditional expression as part of an if or else statement.

I would be happy with prefer-conditional-operator or prefer-ternary-expression

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about prefer-conditional-expression? if and ? ... : are both conditionals, but ? ... : is an expression and if is a statement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, works. The ESTree spec calls it a conditional expression 👍

@adidahiya adidahiya changed the title Add 'prefer-conditional' rule Add prefer-conditional-expression rule May 15, 2017
@adidahiya adidahiya added this to the TSLint v5.3 milestone May 15, 2017
@adidahiya adidahiya merged commit ee1ae25 into palantir:master May 15, 2017
@andy-hanson andy-hanson deleted the prefer-conditional branch May 16, 2017 02:15
@lambda-fairy
Copy link

Is this rule documented anywhere? I don't see it in the rules list.

@mwld
Copy link
Contributor

mwld commented May 30, 2017

Thanks for new awesome rules with the 5.3.0 release! As @lfairy said it would be great to have the new rules listed in the docs with the release.
Even the release note is currently linking to non-existent rule URLs:

I also recognized that most of the mentioned [new-rule-option]s there are not yet documented on their doc pages. E.g.:

Shouldn't it be mandatory to update the docs with rule changes?

@ajafff
Copy link
Contributor

ajafff commented May 30, 2017

The docs are located in a different branch that needs to be updated when releasing a new version. Seems like this was forgotten? @nchen63 @adidahiya

@adidahiya
Copy link
Contributor

yep, thanks for the heads up, looks like we missed updating the docs for 5.3. we'll update them today!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants