-
-
Notifications
You must be signed in to change notification settings - Fork 471
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
New unneeded ternary sniff #1771
Conversation
Seems like travis is failing because of the addition of this sniff 😂 WordPress-Coding-Standards/bin/class-ruleset-test.php Lines 60 to 62 in 80e82a2
|
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.
@dingo-d Hi Denis!
I've had a initial look and here are a couple of things which come to mind:
- This sniff doesn't belong in the
PHP
category, but in theCodeAnalysis
category. - The name of the sniff feels like broken English.
I'm not sure what would be a better name, but here are some suggestions:UnnecessaryTernary
BooleanTernaryAssignment
- From a readability perspective, I'm really not a fan of writing code the way this sniff recommends.
Readability-wise, the preferred way of writing this code would be:Or alternatively:$is_true = false; if ( $var === 'a' ) { $is_true = true; }
$is_true = ( $var === 'a' );
- If you're looking for some more tests, try running the sniff over a number of packages, including WP Core to see what it throws up.
- The PR does not add the sniff to a ruleset.
- This logic used by the sniff is far too simplistic IMO.
- You're not checking that the result is actually being assigned to a variable.
- Just looking at the first/last token of the else/then clauses is very prone to false positives.
- The end of statement determination is making presumptions.
I can think of so many ways to break this sniff.... so let me just throw some unit test cases at you and we can talk again once you've had a chance to look at those.
$test = $a !== $b ? true === $bar : $foo !== false;
$test = $a !== $b ? true === $bar ? 'foo' : 'bar' : $foo !== false;
$test = $a === $b ? true == function( $v ) { return strpos( $v, 'a' ) !== false; }() : 'something else';
// Found in WP core:
if ($this->ipath !== '' &&
(
$isauthority && $this->ipath[0] !== '/' ||
(
$this->scheme === null &&
!$isauthority &&
strpos($this->ipath, ':') !== false &&
(strpos($this->ipath, '/') === false ? true : strpos($this->ipath, ':') < strpos($this->ipath, '/'))
)
)
) {
return false;
}
Seems like travis is failing because of the addition of this sniff
So, have you looked at how to fix that ?
@@ -0,0 +1,19 @@ | |||
<documentation title="Discourage the usage of the unneeded ternary operator"> |
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 should just be a title, not a description.
* | ||
* This sniff is influenced by the same rule set in the ESLint rules. | ||
* When we want to store a boolean value in a variable, using a ternary operator | ||
* is not needed, because a test will return a boolean value. |
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.
* is not needed, because a test will return a boolean value. | |
* This sniff is influenced by the same rule as found in the ESLint rules. | |
* When we want to store the boolean result of a comparison in a variable, using | |
* a ternary operator with `? true : false` is not needed, because the comparison | |
* will return a boolean value. |
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'll rephrase this description so that it's not only limited to variable assignments (as is the initial intention of the sniff) 👍
class NoUnneededTernarySniff extends Sniff { | ||
|
||
/** | ||
* Boolean tokens array. |
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.
* Boolean tokens array. | |
* Tokens which represent booleans. |
|
||
if ( isset( $this->boolean_tokens[ $first_token_type ] ) && isset( $this->boolean_tokens[ $last_token_type ] ) ) { | ||
$this->phpcsFile->addError( | ||
'Don\'t use ternary epression to store a boolean value in a variable.', |
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'm still not happy with this phrase as IMO it's still unclear.
'Don\'t use ternary epression to store a boolean value in a variable.', | |
'Don\'t use a ternary expression to store the boolean result of a comparison in a variable.', |
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 ESLint will throw Unnecessary use of boolean literals in conditional expression
, can we use this instead of the proposed message, or make something better?
$this->phpcsFile->addError( | ||
'Don\'t use ternary epression to store a boolean value in a variable.', | ||
$stackPtr, | ||
'Detected' |
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 default error code is Found
, which works perfectly well here. Why use something else here and make the error code less predictable ?
'Detected' | |
'Found' |
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.
Wasn't aware of the Found
vs Detected
. I guess I saw the error message of some other sniff having Detected
so I used it. Will fix this 👍
$last_token_type = $this->tokens[ $last_token ]['type']; | ||
|
||
if ( isset( $this->boolean_tokens[ $first_token_type ] ) && isset( $this->boolean_tokens[ $last_token_type ] ) ) { | ||
$this->phpcsFile->addError( |
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 don't think this warrants a hard error which breaks build. A soft warning should be enough.
$this->phpcsFile->addError( | |
$this->phpcsFile->addWarning( |
* Discourage the usage of the unneeded ternary operator. | ||
* | ||
* This sniff is influenced by the same rule set in the ESLint rules. | ||
* When we want to store a boolean value in a variable, using a ternary operator |
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.
Should this sniff also cover return ... ? true : false;
?
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 think this is the point of the sniff. Wherever there is an unneeded ternary detected, it can be removed. This includes conditionals, return statements or assignment to a variable 👍
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.
And yet, there are no unit tests which shows that return statements are covered by this sniff.
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.
Will add those when I update the sniff 👍
I'll close this PR here as the sniff is not WordPress specific, so I'll open an issue in the PHPCSExtra to add this sniff there (and I'll probably start working on it there). |
This sniff addresses #1291 issue.
When doing an assignment to a boolean with a ternary, if the ternary is only containing boolean values, this is an overhead, as a simple test will yield a boolean.
Includes unit tests.
Includes documentation.
I have added some unit tests, but I'd like to see if some more extra cases need to be addressed.