Skip to content

docs: expand PR review process with package-PR checklist#46

Merged
markshust merged 1 commit into
developfrom
feature/pr-review-doc-update
May 1, 2026
Merged

docs: expand PR review process with package-PR checklist#46
markshust merged 1 commit into
developfrom
feature/pr-review-doc-update

Conversation

@markshust
Copy link
Copy Markdown
Collaborator

Summary

Refreshes .claude/pr-review-process.md with the gaps surfaced during the marko/vite review (#42) so future contributor PRs get a complete first-pass review.

What's new in .claude/pr-review-process.md

  • Package-PR-specific checks (new section 5a):
    • module.php is optional, and minimal when used (defaults: enabled: true, list-style singletons trigger autowiring directly — explicit bindings: [X::class => X::class] entries are redundant)
    • composer.json conventions: type: marko-module, no version, self.version for internal deps, 4-space indent
    • Full file structure for a new package + cross-cutting updates (root composer.json, both ISSUE_TEMPLATE dropdowns, IntegrationVerificationTest/PackagingTest count bumps)
    • Required docs page at docs/src/content/docs/packages/{name}.md per docs/DOCS-STANDARDS.md
  • Loud errors / no silent fallbacks explicitly listed (no swallowed exceptions, no HTML-comment returns, no default-on-missing-config)
  • No defensive instanceof checks on container resolutions
  • No pseudo-functionality review item
  • No PR CI workflow exists — merge-readiness signal documented (local lint/tests + mergeStateStatus: CLEAN)
  • PR comment drafting (new step 7): always show comment drafts to the user for approval before posting
  • Merge policy clarified (step 8): --merge, not --squash. Co-Authored-By: trailers go on maintainer fix commits, not the merge
  • Pre-merge checklist expanded with package-specific items

CLAUDE.md

Adds a "Reviewing PRs" section pointing at .claude/pr-review-process.md as a lookup-on-demand reference (not auto-imported with @) so it doesn't consume context on non-review sessions. The .claude/ configuration list now includes all the existing files in that directory.

Test plan

  • Documentation only — no code changes
  • Verified that the lessons captured here match what was actually pushed back during the marko/vite review

🤖 Generated with Claude Code

…licy

Updates `.claude/pr-review-process.md` based on lessons from PR #42 (marko/vite):

- Adds package-PR-specific checks (5a): module.php is optional and minimal,
  composer.json conventions, full new-package file structure, cross-cutting
  updates (issue-template dropdowns, test-count bumps, required docs page
  at docs/src/content/docs/packages/{name}.md)
- Adds explicit "loud errors / no silent fallbacks" guidance
- Adds "no defensive instanceof checks on container resolutions"
- Adds "no pseudo-functionality" review item
- Notes that no PR CI workflow exists; the merge-readiness signal is local
  lint/tests + mergeStateStatus CLEAN
- Adds step 7 for drafting PR comments before posting
- Reaffirms --merge (not --squash) as the merge strategy and explains
  Co-Authored-By trailers go on maintainer fix commits, not the merge
- Expands the pre-merge checklist with package-specific items

Adds a "Reviewing PRs" section to CLAUDE.md that references the doc as
lookup-on-demand (not @ imported) so it loads only when relevant. Expands
the .claude/ configuration list to include all existing files.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label May 1, 2026
@markshust markshust merged commit 5bfeb45 into develop May 1, 2026
1 check passed
@markshust markshust deleted the feature/pr-review-doc-update branch May 1, 2026 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant