refactor(typeutil): split map-extraction helpers into lookup.go - #49746
Conversation
Move ParseBool, LookupMap, LookupString, and LookupStringPath into a new pkg/typeutil/lookup.go. Update convert.go package doc to focus on numeric conversions. Update README.md with a dedicated Map Extraction section. Closes #49698 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors typeutil by separating map extraction from numeric conversion.
Changes:
- Moves map lookup helpers into
lookup.go. - Narrows
convert.goto numeric conversions. - Expands package documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/typeutil/convert.go |
Removes map helpers and updates documentation. |
pkg/typeutil/lookup.go |
Houses map extraction helpers. |
pkg/typeutil/README.md |
Documents the revised API layout. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. Changes are refactoring of pkg/typeutil package (README.md, convert.go, and new lookup.go) with no new or modified test files. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — three actionable issues found; requesting changes.
📋 Key Themes & Highlights
Key Themes
- Test file not co-located:
TestLookupMap,TestLookupString, andTestLookupStringPathremain inconvert_test.go, leavinglookup.gowithout a correspondinglookup_test.go. The refactor is incomplete. - Package doc narrowed incorrectly:
convert.go's package comment now says single-value
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19 AIC · ⌖ 18.7 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
Review: refactor(typeutil): split map-extraction helpers into lookup.go
Clean structural refactor with no behavior changes. Two pre-existing comments already capture the actionable gaps; no additional blocking issues found.
Existing open issues (not duplicated here)
convert_test.gostill owns the lookup tests —TestLookupMap,TestLookupString,TestLookupStringPathbelong in a newlookup_test.goto complete the split.- Package doc narrowed too aggressively —
convert.goline 1 now says "single-value numeric conversion utilities" but this is Go's package-level synopsis for the wholetypeutilpackage; it should still describe the full package.
Addressing these two existing comments will finish the refactor properly.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.5 AIC · ⌖ 9.27 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Verdict: non-blocking, but the refactor introduces the same class of drift it set out to fix.
Pure reorganization with no logic changes — confirmed by diffing old/new function bodies (ParseBool, LookupMap, LookupString, LookupStringPath are byte-identical modulo location). No correctness, concurrency, or security issues found.
💡 Themes
- Duplicate package doc comments: both
convert.goandlookup.gonow start with// Package typeutil ..., which Go/go doc/pkg.go.dev only expect once per package — ironic given this PR's premise is fixing documentation drift. - Test file split left incomplete:
TestLookupMap,TestLookupString,TestLookupStringPathremain inconvert_test.goeven though their functions moved tolookup.go; nolookup_test.gowas created, soconvert_test.gostill mixes both concerns. - Documentation content itself (README, function doc comments) is otherwise accurate and consistent with the new layout.
Neither issue affects behavior, so this can be addressed as a quick follow-up rather than blocking merge.
🔎 Code quality review by PR Code Quality Reviewer · auto · 36.2 AIC · ⌖ 4.1 AIC · ⊞ 7.8K
Comment /review to run again
…nto lookup.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (142 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot Please address the open review feedback on this PR, then run the pr-finisher skill. Outstanding review signal: CHANGES_REQUESTED. Key follow-ups observed:
After updates, re-run formatting as needed and push the fixes to this branch.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in 9717fa2. I moved lookup tests into |
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "defer",
"category": "refactor",
"pr_number": 49746,
"risk": "medium"
}
|
I verified the branch state and addressed the remaining actionable review state by replying on each still-open in-scope review thread (Copilot reviewer + github-actions bot) with the exact fix references already landed in |
|
@copilot Please verify whether this PR is now ready for merge, then run the pr-finisher skill. Current signals:
Branch refresh was requested for this PR. After verification, run the pr-finisher skill. Run details: https://github.com/github/gh-aw/actions/runs/30750494960
|
Verified on current head |
|
@copilot Unresolved review threads to address (newest first):
I also attempted a branch refresh. Please address the items above, rerun checks as needed, and then continue with the
|
Verified on current head |
|
@copilot Please continue triage on this PR. Current maintainer-visible follow-up:
I also requested a branch refresh for this PR. Run details: https://github.com/github/gh-aw/actions/runs/30753985170
|
Verified on current head Validation/triage this pass:
From the agent side, review-thread closure work is complete; remaining merge gate is reviewer/maintainer clearance plus a human-triggered CI run on the latest head if required by policy. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in Root cause from job I updated that model pin and recompiled, which updated both:
Validation run locally:
|
|
🎉 This pull request is included in a new release. Release: |
convert.gomixed two unrelated concerns: single-value numeric conversion and keyedmap[string]anyextraction. The four map-extraction functions (ParseBool,LookupMap,LookupString,LookupStringPath) were undocumented in the file's own package doc, creating visible drift.Changes
pkg/typeutil/lookup.go(new) — receivesParseBool,LookupMap,LookupString,LookupStringPathwith a focused file-level doc block describing the map-extraction clusterpkg/typeutil/convert.go— removes the four moved functions; narrows its package doc to numeric-only conversions; adds a cross-reference tolookup.gopkg/typeutil/README.md— adds a dedicated Map Extraction section, extends the "Choosing the Right Function" table, updates the overview and design notes to reflect the two-file layoutPure reorganization — no signature or behavior changes; all callers and tests are unaffected.
Run: https://github.com/github/gh-aw/actions/runs/30750494960
run: https://github.com/github/gh-aw/actions/runs/30751969165
"$BODY_49746"