Reduce fetchAndSaveRemoteIncludesWithOptions parameter count with options struct - #54407
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54407 does not have the 'implementation' label and has only 56 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #54407. No actionable blocking issues found in the diff; no PR comments or review submission needed.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — approving with one minor observation.
📋 Key Themes & Highlights
Key Themes
- Struct introduction is well-scoped:
includesFetchOptionsmirrors the existingfrontmatterImportsOptspattern in the same file, fitting the established convention. - Recursive call is cleaner: Passing
optsdirectly removes 7 positional arguments that were previously easy to mis-order. - Zero-value booleans in tests: Updated tests correctly omit
verbose,force, andstrict, taking advantage of Go zero-values.
One minor observation
opts.fetchFn is mutated in the local copy at the top of the function to default to FetchIncludeFromSource. Because opts is passed by value this is safe and correct, but a small inline comment (e.g. // opts is a value copy; mutation is intentional so recursive calls inherit the resolved fetchFn) would make the intent clear to future readers.
Positive Highlights
- ✅ Behaviour is demonstrably unchanged — the refactor is mechanical.
- ✅ The struct comment explains why
contentis the only varying parameter. - ✅
fetchAllRemoteDependenciesWithOptionscall site uses named fields, making thefetchFn: nilomission explicit via zero-value convention.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 15.6 AIC · ⌖ 9.85 AIC · ⊞ 7.8K
Comment /matt to run again
There was a problem hiding this comment.
Pull request overview
Refactors remote include fetching to satisfy the parameter-count limit without changing behavior.
Changes:
- Introduces
includesFetchOptions. - Updates recursive and test call sites.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/includes.go |
Consolidates constant fetch parameters into an options struct. |
pkg/cli/remote_workflow_test.go |
Adapts existing tests to the new signature. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Clean refactoring — replacing 9 positional parameters with an options struct improves call-site readability and makes future additions easier. The opts.fetchFn nil-check mutates a local copy (value receiver), so the default is applied correctly without affecting callers. Recursive calls pass the already-populated opts, which is correct. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.4 AIC · ⌖ 8.78 AIC · ⊞ 5.7K
|
🎉 This pull request is included in a new release. Release: |
LintMonster flagged
fetchAndSaveRemoteIncludesWithOptionsinpkg/cli/includes.gofor exceeding the 9-parameter limit (max 8).Refactor
includesFetchOptionsstruct to consolidate the constant parameters (spec,targetDir,verbose,force,tracker,fetchFn,strict), mirroring the existingfrontmatterImportsOptspattern already used elsewhere in the same file.fetchAndSaveRemoteIncludesWithOptionssignature from 9 params down to(ctx, content, opts).fetchAllRemoteDependenciesWithOptionsto construct/passincludesFetchOptions.remote_workflow_test.goto use the new signature.Behavior is unchanged; the change is scoped strictly to the parameter-count finding and does not touch unrelated
largefuncor mutable-state findings in the same package.