Skip to content

refactor(init): address PR #124 review feedback#155

Merged
wyattjoh merged 1 commit intomainfrom
refactor/format-review-feedback
Apr 14, 2026
Merged

refactor(init): address PR #124 review feedback#155
wyattjoh merged 1 commit intomainfrom
refactor/format-review-feedback

Conversation

@wyattjoh
Copy link
Copy Markdown
Contributor

Summary

Follow-up to #124 addressing review feedback that landed after merge.

  • Drop the commit-message-style comment above the runners import; the function docstring and import names already cover the contract.
  • Simplify runFormatters to trust ctx.deps. gatherContext already calls readDeps, so the readDeps(ctx.cwd) fallback was dead code in practice and masked intent.
  • Drop the silent try/catch blocks around Bun.spawn/which/spawnSync mock setters in format.test.ts. They swallowed mock-assignment failures; if Bun ever makes those properties non-writable, tests would run against real globals with confusing results. Now verify the assignment took effect and fail fast.
  • Extract the Bun override cast once to cut boilerplate.
  • Add a test locking in the best-effort behavior when a formatter exits non-zero (we intentionally ignore exit codes).

Test plan

  • bun run test passes

- Drop commit-message-style comment above runners import; the docstring
  and import names cover the contract.
- Trust ctx.deps in runFormatters; gatherContext already reads deps from
  disk, so the fallback readDeps call was dead code in practice.
- Drop try/catch blocks around Bun.spawn/which/spawnSync mock setters;
  verify the assignment took effect instead, so a future non-writable
  property fails fast rather than silently running against real globals.
- Extract the Bun override cast once to reduce boilerplate.
- Add test locking in best-effort behavior when a formatter exits non-zero.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dd328731-3eea-4f65-8057-ae6767267dca

📥 Commits

Reviewing files that changed from the base of the PR and between 8812855 and f9ed574.

📒 Files selected for processing (2)
  • packages/cli-core/src/commands/init/format.test.ts
  • packages/cli-core/src/commands/init/format.ts

📝 Walkthrough

Walkthrough

The changes remove disk-based dependency discovery from the formatter initialization logic. The production code (format.ts) eliminates the readDeps() call and adds an early return when ctx.deps is empty, forcing exclusive reliance on ctx.deps. The test file (format.test.ts) restructures its Bun API mocking to use a single typed bunOverrides view with explicit error throwing on failed assignments, replaces mock restore logic with direct reassignment, and removes test cases that depended on disk-based package discovery.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly references PR #124 review feedback refactoring, which matches the changeset that addresses review comments on formatter logic and test mocking improvements.
Description check ✅ Passed The description is directly related to the changeset, detailing all major modifications including simplification of runFormatters, removal of try/catch blocks, mock setup improvements, and test additions for best-effort behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@wyattjoh wyattjoh merged commit 8068c75 into main Apr 14, 2026
9 checks passed
@wyattjoh wyattjoh deleted the refactor/format-review-feedback branch April 14, 2026 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants