Update flip sniffs to handle object properties better#2839
Conversation
📝 WalkthroughWalkthroughThe PR enhances comparison operator flipping logic in five PHP CodeSniffer sniffs. Each sniff receives a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnVariableSniff.php (1)
600-637: Consider handling bitshift operators<<and>>.The helper correctly avoids arrow tokens (
->,=>) and multi-character comparisons (<=,>=). However, bitshift operators like$x << 2or$x >> 2would be incorrectly matched as comparison operators since<before<isn't in the skip list.This may be a rare edge case in conditions, but could cause incorrect flips for expressions like
$flags >> 1.🔧 Suggested fix
// Check character after to avoid matching <=, >=, =>. if ( $pos < strlen( $condition ) - 1 ) { $charAfter = $condition[ $pos + 1 ]; - if ( $charAfter === '=' || $charAfter === '>' ) { + if ( $charAfter === '=' || $charAfter === '>' || $charAfter === '<' ) { ++$pos; continue; } }And add
<to the before-check as well to handle cases like$x << 1where we're looking for>:// Skip if this is part of ->, =>, <=, >=, or a multi-char comparison. - if ( $charBefore === '-' || $charBefore === '=' || $charBefore === '<' || $charBefore === '>' || $charBefore === '!' ) { + if ( $charBefore === '-' || $charBefore === '=' || $charBefore === '<' || $charBefore === '>' || $charBefore === '!' || $charBefore === $operator ) {
🧹 Nitpick comments (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipLoopIfElseToContinueSniff.php (1)
424-461: Consider extracting shared helper methods into a trait.The
findComparisonOperator(),flipComparisonOperator(),hasTopLevelOperator(),negateCondition(),dedentCode(), andgetIndentation()methods are duplicated across all fiveFlip*Sniffclasses. Consolidating them into a trait (e.g.,ConditionFlippingTrait) would reduce maintenance burden and ensure consistent behavior.♻️ Example trait structure
<?php namespace Formidable\Sniffs\CodeAnalysis; trait ConditionFlippingTrait { private function findComparisonOperator( $condition, $operator ) { // shared implementation } private function flipComparisonOperator( $condition ) { // shared implementation } private function hasTopLevelOperator( $condition ) { // shared implementation } private function negateCondition( $condition ) { // shared implementation } private function dedentCode( $code ) { // shared implementation } private function getIndentation( File $phpcsFile, $stackPtr ) { // shared implementation } }Then in each sniff class:
class FlipLoopIfElseToContinueSniff implements Sniff { use ConditionFlippingTrait; // ... }
…_object_properties_better Updae flip sniffs to handle object properties better
Some conditions would change to something like
if ( empty( $body - <= success ) ) {, which is totally invalid.Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.