Skip to content

Conversation

@jmgilman
Copy link
Collaborator

@jmgilman jmgilman commented Jan 2, 2026

This PR addresses several code quality issues identified during pre-release review:

DRY Refactoring:

  • Extract shared container runtime logic into common.go (baseRuntime struct)
  • Consolidate duplicate gitError helper functions in git.go
  • Remove redundant Config struct methods (IsValidAgent, ValidAgentNames, etc.) in favor of existing package-level functions

Error Handling Improvements:

  • Replace direct error comparisons with errors.Is() throughout:
    • instance/manager.go: catalog.ErrNotFound checks
    • keychain/keychain_darwin.go: gokeychain.ErrorItemNotFound checks
  • Document intentional error skipping in Manager.List

Documentation Improvements:

  • Add package documentation to cmd package
  • Improve GlobalMRUSession struct documentation
  • Add explanatory comment for sanitizeBranch test

Code Reduction:

  • ~250 lines removed through deduplication
  • container/podman.go and container/apple.go significantly simplified

This PR addresses several code quality issues identified during pre-release review:

**DRY Refactoring:**
- Extract shared container runtime logic into common.go (baseRuntime struct)
- Consolidate duplicate gitError helper functions in git.go
- Remove redundant Config struct methods (IsValidAgent, ValidAgentNames, etc.)
  in favor of existing package-level functions

**Error Handling Improvements:**
- Replace direct error comparisons with errors.Is() throughout:
  - instance/manager.go: catalog.ErrNotFound checks
  - keychain/keychain_darwin.go: gokeychain.ErrorItemNotFound checks
- Document intentional error skipping in Manager.List

**Documentation Improvements:**
- Add package documentation to cmd package
- Improve GlobalMRUSession struct documentation
- Add explanatory comment for sanitizeBranch test

**Code Reduction:**
- ~250 lines removed through deduplication
- container/podman.go and container/apple.go significantly simplified
@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
headjack 8f0288f Commit Preview URL

Branch Preview URL
Jan 02 2026, 11:05 PM

@jmgilman jmgilman merged commit fe47a84 into master Jan 2, 2026
2 checks passed
@jmgilman jmgilman deleted the claude/review-cli-code-nVYTU branch January 2, 2026 23:11
jmgilman added a commit that referenced this pull request Jan 3, 2026
Current behavior:
Code contained duplicate runtime logic across container implementations, redundant Config struct methods, direct error comparisons instead of errors.Is(), and missing package documentation

New behavior:
Shared container logic extracted to baseRuntime struct, duplicate gitError functions consolidated, redundant Config methods removed in favor of package-level functions, error handling improved with errors.Is() throughout codebase, and comprehensive documentation added

Closes: #32
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.

3 participants