-
Notifications
You must be signed in to change notification settings - Fork 889
Add 'no-boolean-literal-in-conditional' rule #2534
Add 'no-boolean-literal-in-conditional' rule #2534
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule is only usefule when compiling with strictNullChecks. That should be documented.
The rule could even check the compiler option and print a warning if necessary
@@ -86,7 +86,7 @@ function fix(node: ts.BinaryExpression, { negate, expression }: Compare): Lint.F | |||
} | |||
} | |||
|
|||
function needsParenthesesForNegate(node: ts.Expression): boolean { | |||
export function needsParenthesesForNegate(node: ts.Expression): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you really need to share this function, move it to utils
return undefined; | ||
} | ||
|
||
if (l !== undefined && r !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the following lines need some refactoring really badly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO those conditional expressions look really nasty. It would help clarity to convert them to if ... else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already written as an if ... else if ...
chain.
const l = getLiteralBoolean(whenTrue); | ||
const r = getLiteralBoolean(whenFalse); | ||
// Perf: only check type of condition if there would be a failure otherwise | ||
if ((l !== undefined || r !== undefined) && !isBoolean(condition, checker)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess isBoolean()
assumes strictNullChecks: true
?
It's useful without |
@andy-hanson please update this branch so we can review it, or close it if no longer relevant. we will close this if we do not hear from you in two weeks. @ajafff is this still worth it or should we just close? |
Closing due to age and inactivity. Feel free to re-open or create a new pull request if you decide to continue working on this. |
PR checklist
Overview of change:
Added the
no-boolean-litearl-in-conditional
rule, which forbidsx ? true : y
and the like.CHANGELOG.md entry:
[new-rule]
no-boolean-literal-in-conditional