New sniff to simplify ternaries that check the opposite thing in the …#2796
Conversation
…2nd condition group
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Helper ternary refactors classes/helpers/FrmAppHelper.php, classes/helpers/FrmFieldsHelper.php, classes/models/FrmFieldFormHtml.php |
Replaced compound boolean expressions with simpler ternary-based returns/assignments while preserving existing behavior (array vs scalar checks, and description-shortcode replacement condition). |
New CodeSniffer sniff phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyDualConditionToTernarySniff.php |
New sniff detecting patterns of two parenthesized groups joined by ` |
PHPCS ruleset update phpcs.xml |
Registers the new Formidable.CodeAnalysis.SimplifyDualConditionToTernary sniff in the ruleset. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- Remove more useless parentheses #2767: Touches the same helper files and simplifies boolean/parenthesized expressions, making it likely related to these refactors.
Poem
🐇 I hop through code in morning light,
Hunting || and && with delight,
Where opposites meet in parenthesis' lair,
I stitch one ternary with careful care,
Hooray — less branching, tidier air! ✨
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The PR title references 'ternaries that check the opposite thing' but the actual implementation creates a new sniff that simplifies dual-condition patterns (A && B) || (!A && C) to ternary form. The title is truncated ('…') and partially misleading about what the sniff does. | Complete the title and clarify that it adds a sniff to detect and simplify dual-condition boolean patterns to ternaries, e.g., 'Add sniff to simplify dual-condition boolean expressions to ternaries'. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧹 Recent nitpick comments
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyDualConditionToTernarySniff.php (2)
324-347: Consider tracking additional bracket types for robustness.The depth tracking only accounts for parentheses. Edge cases with array syntax or closures inside conditions (e.g.,
(fn() => true && ...) || ...) could potentially cause incorrect&&detection.This is a minor edge case for typical usage patterns, but for completeness, consider also tracking
T_OPEN_SQUARE_BRACKET/T_CLOSE_SQUARE_BRACKETandT_OPEN_CURLY_BRACKET/T_CLOSE_CURLY_BRACKET.
60-62: Note:strpos,stripos, andpreg_matchreturnint|false, not boolean.These functions are commonly used in boolean contexts, so including them is reasonable for practical purposes. However, transforming expressions using these may slightly alter semantics since
0(valid match at position 0) is falsy but distinct fromfalse. This is likely acceptable for the intended use cases.
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyDualConditionToTernarySniff.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (3)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyDualConditionToTernarySniff.php (3)
357-365: LGTM!The logic correctly verifies that exactly one condition is negated and that both conditions have identical content.
376-460: LGTM!The boolean expression validation is thorough: it checks for comparison operators at the top level and verifies against the known boolean-returning functions list, including negated function calls.
471-480: LGTM!Simple and correct token content extraction.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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 @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
classes/models/FrmFieldFormHtml.php (1)
213-220: Looks behavior-preserving; consider simpler boolean equivalence for readability.
$should_replace = $is_html ? $wp_processed : ! $wp_processed;is equivalent to$is_html === $wp_processed, which may be easier to scan in the future.Proposed tweak
- $is_html = 'html' === $this->field_obj->get_field_column( 'type' ); - $should_replace = $is_html ? $wp_processed : ! $wp_processed; + $is_html = 'html' === $this->field_obj->get_field_column( 'type' ); + $should_replace = ( $is_html === $wp_processed );phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyDualConditionToTernarySniff.php (2)
139-180: Consider skipping comments (not just whitespace) inside groups to improve match rate.
getFirstCondition/getRestOfConditioncurrently only skipT_WHITESPACE, so inline comments between tokens can prevent detection (false negatives). UsingTokens::$emptyTokensconsistently would make this more resilient.Also applies to: 193-237
281-289: Normalize condition text before comparison to reduce false negatives.
areOppositeConditionsrequires exact string equality for$cond['content']; small formatting differences (extra parens/whitespace) will miss otherwise-valid cases. Consider normalizing whitespace and optionally stripping redundant outer parens before comparing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/helpers/FrmAppHelper.phpclasses/helpers/FrmFieldsHelper.phpclasses/models/FrmFieldFormHtml.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyDualConditionToTernarySniff.phpphpcs.xml
🧰 Additional context used
🧬 Code graph analysis (1)
classes/helpers/FrmFieldsHelper.php (1)
classes/helpers/FrmAppHelper.php (1)
checked(2419-2423)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (3)
classes/helpers/FrmAppHelper.php (1)
2431-2438: LGTM: clearer control flow with the ternary.This keeps the same behavior while being more compact.
classes/helpers/FrmFieldsHelper.php (1)
679-709: LGTM: simplified default-value check without changing behavior.The ternary keeps the same “array vs scalar” semantics and reads cleaner inline.
phpcs.xml (1)
240-261: Sniff reference is valid. TheSimplifyDualConditionToTernarysniff file exists atphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyDualConditionToTernarySniff.phpwith the correct class definition, and PHPCS will resolve it without errors via the dealerdirect composer plugin's automaticinstalled_pathsconfiguration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…ies_that_check_the_same_thing_twice_but_opposite New sniff to simplify ternaries that check the opposite thing in the …
…2nd condition group
The sniff detects (A && B) || (!A && C) patterns and converts them to A ? B : C ternary expressions.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.