Repository navigation
fix: preserve consumer parameter syntax by default - #65
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Configure the default policy src/Config/ECSConfig.php, tests/Fixtures/preset-rules.json, docs/resolved-preset.md, .changelog/conservative-parameter-commas.md |
The default excludes parameter declarations from the trailing-comma elements while retaining the other configured elements. Documentation describes the PHP 8.0 syntax requirement, existing comma preservation, and explicit consumer configuration. |
Verify default and override behavior tests/Fixtures/consumer-language.php.inc, tests/Integration/StandardsTest.php |
Integration tests check default formatting and an explicit parameter-comma override. They also verify repeated check/fix runs leave the formatted result unchanged. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related issues
- Issue 1: This change addresses part of the consumer PHP compatibility policy by excluding parameter commas from the default.
- Issue 5: The integration tests add resolved-rule behavior and idempotence coverage, but do not provide a full rule-by-rule policy comparison.
- Issue 35: The change documents a parameter-list policy difference, but does not adopt PER3.
Possibly related PRs
- php-fast-forward/code-standards#29: This PR updates
docs/resolved-preset.md; the RST replacement would need to retain its parameter-comma documentation. - php-fast-forward/code-standards#7: This PR changes the preset and documents configuration introduced by that PR.
Merge Risk: 
b8908
-
The default avoids introducing parameter commas while leaving explicit opt-in available. No actionable merge-blocking risk is identified from the supplied evidence.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: preserving consumer parameter syntax by default. Description check The description is directly related to the changeset and explains the compatibility issue, configuration change, override behavior, testing, and scope. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 2 files. (4 skipped: 4 unsupported.)
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks each comma’s place
No host-born mark can change its face
The default leaves parameters bare
An opt-in adds commas with care
Check and fix return the same
The rabbit hops, content with the game
Comment @coderabbitai help to get the list of available commands.
Changelog✅ Contribution check passed. Head: Base: Contribution: 1 new valid fragment(s). Policy: ordinary.
|
|
@codex review Please review exact draft HEAD b8908ae. This bounded compatibility correction removes only native parameter trailing commas from the default, preserving callback-last overrides and existing syntax. Independent final source review and full local composer check passed123tests/840assertions; hosted PHP8.3/8.4/8.5, reusable check, Changelog and CodeQL are successful. No existing Codex request or running marker is present for this HEAD. The PR remains draft pending real external gates; this does not request a merge, tag or release. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@coderabbitai review Please review exact draft HEAD b8908ae. This is the first CodeRabbit request on this PR; the automatic draft skip is not approval and no review is active here. The prior repository quota response at05:20 estimated another included review after46minutes; it is now after06:10UTC. This request uses only included capacity and does not authorize credits or configuration changes. The six-path correction omits only native parameter-list commas from the default so the formatter host does not raise consumer PHP syntax. Existing commas and explicit callback-last replacement remain preserved. Qualified RED/GREEN, full local123tests/840assertions, current hosted PHP8.3/8.4/8.5/reusable/Changelog/CodeQL and exact-head Codex code/security are successful. No consumer mutation, blocked multiline code, merge, tag or release. |
✅ Action performed
|
Refs #2 and #5: this corrects one confirmed consumer-language regression; target selection and the wider consumer/PHP dependency matrices remain partial, so neither issue is closed.
A real error-handler consumer declares PHP ^7.4 || ^8.0. Running published Code Standards v0.3.2 on PHP8.5 suggested trailing commas in parameter declarations, which require PHP8.0. The formatter host does not authorize raising the consumer's syntax floor.
Omit only parameters from native TrailingCommaInMultilineFixer default elements. Preserve after_heredoc, arguments, arrays, destructuring and match; existing parameter commas remain untouched. The native ECS callback still runs last and can explicitly replace the configuration after a consumer reviews PHP8-or-later compatibility. No new target-version parser, CLI option, dependency or consumer mutation. This conservative exception is not global PHP7.4 certification or an unconditional PER conformance claim.
Commit b8908ae, direct base648c57aa6843d01b11f02a4cf163b1865f9e6a1b; six independently reviewed paths. No dependency/lock update or code from the blocked multiline candidate.
Validation: exact-lock native installation51packages/zero updates on PHP8.5.12/ECS13.3.9/Rector2.4.2/PHPStan2.1.51. Qualified RED reproduced the parameter-comma failure; GREEN preset+CLI3tests/158assertions. Review strengthened the override proof to require absence of array/argument commas and exact violation exit2; final focused2tests/28assertions passed. Full CI=true GITHUB_ACTIONS=true composer check passed123tests/840assertions, Composer strict/platform and own35PHP syntax/ECS check. Tests cover typed/nullable parameters, existing commas, arguments/arrays, new-expression parentheses, explicit opt-in override, check without writes, explicit fix, warm-cache and idempotence. No PHP7.4 runtime was executed. Hosted matrix and current-head external reviews remain gates.
The consumer pilot remained unchanged, including its65preexisting tracked diffs, index, untracked/ignored entries, manifest and lock. A patch fragment records this focused correction. No merge, tag or release.