Uh oh!
There was an error while loading. Please reload this page.
refactor: extract helpers to bring 3 functions under the 60-line largefunc limit - #42624
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…thub, parser Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (95 additions detected, threshold is 100). |
There was a problem hiding this comment.
Pull request overview
This PR primarily refactors three Go functions that exceeded the custom largefunc (60-line) limit by extracting focused private helpers, aiming to keep behavior unchanged. It also includes a GitHub Actions workflow change that alters actions/checkout behavior and should be either justified in the PR description or reverted.
Changes:
- Extracted helper functions in
pkg/console,pkg/github, andpkg/parserto reduce function length under thelargefuncthreshold. - Introduced a documentation mismatch in
computeValueFirst(comment describes priority-order evaluation that the code does not implement). - Updated
.github/workflows/agentics-maintenance.ymlto setclean: falseon multipleactions/checkoutsteps (behavioral change not mentioned in the PR description).
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/schema_suggestions.go | Extracts exact/fuzzy matching loops into helper functions for schema field suggestions. |
| pkg/github/label_objective_mapping.go | Extracts per-logic branches (sum/first/max) into helper methods. |
| pkg/console/console.go | Extracts the table StyleFunc closure into buildTableStyleFunc. |
| .github/workflows/agentics-maintenance.yml | Changes checkout behavior by disabling workspace cleaning in multiple jobs. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 10
- Review effort level: Low
| // computeValueFirst returns the value for the highest-priority matching label. | ||
| // It walks issueLabels in priority order; if none match, it falls back to the | ||
| // first label in matchingValues. |
| sparse-checkout: | | ||
| actions | ||
| clean: false | ||
| persist-credentials: false |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — two observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Unrelated workflow changes: 8
clean: falseadditions to.github/workflows/agentics-maintenance.ymlare not mentioned in the PR description. Either document the reason or extract to a separate PR. - Implicit preconditions:
computeValueFirstandcomputeValueMaxboth indexmatchingValues[0]without documenting thelen(matchingValues) > 0precondition. Safe today (caller guards at line 56), but worth documenting now that these are standalone methods.
Positive Highlights
- ✅ All three extractions are clean and preserve the original logic exactly
- ✅ Good doc comments on
buildTableStyleFunc,collectExactFieldMatches, andcollectFuzzyFieldMatches - ✅ The
schema_suggestions.gorefactor is the most impressive — 72 → 14 lines with well-named helpers and correct sort/log placement - ✅ Existing tests (
label_objective_mapping_test.go,schema_suggestions_test.go) cover the three extraction paths through the public API, so behavioral regression is guarded - ✅ Minor style cleanup (
struct{}vsstruct {\n}) is a nice bonus
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 51 AIC · ⌖ 7.32 AIC · ⊞ 6.6K
Comment /matt to run again
| with: | ||
| sparse-checkout: | | ||
| actions | ||
| clean: false |
There was a problem hiding this comment.
[/codebase-design] These clean: false additions are unrelated to the stated PR goal (largefunc refactoring) and are not mentioned in the PR description.
💡 Suggestion
Either:
- Add a brief note in the PR description explaining why
clean: falseis needed here, or - Extract these workflow changes into a separate PR for a cleaner commit history.
Keeping unrelated operational fixes separate from refactoring PRs makes bisect and rollback much easier.
@copilot please address this.
| return result | ||
| } | ||
| // Fallback to first matching label | ||
| result := matchingValues[0] |
There was a problem hiding this comment.
[/codebase-design]computeValueFirst (and computeValueMax at line 111) silently require len(matchingValues) > 0 but neither the signature nor the doc comment states this precondition.
💡 Suggestion
The caller (ComputeObjectiveValue) guards against this at line 56, so it is safe today. However, since these are now standalone methods, a future caller could pass an empty slice and get a panic with no obvious explanation. Add the precondition to the doc comment:
// computeValueFirst returns the value for the highest-priority matching label.// It walks issueLabels in priority order; if none match, it falls back to the// first label in matchingValues.// Precondition: len(matchingValues) > 0.func (om*ObjectiveMapping) computeValueFirst(...) int {Same addition applies to computeValueMax.
@copilot please address this.
pelikhan
commented
Jul 1, 2026
@copilot run pr-finisher skill |
…low clean:false changes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 54a5e85. Two issues addressed:
All local checks pass ( |
🎉 This pull request is included in a new release. Release: |
Three functions flagged by the custom
largefunclinter (limit: 60 lines) are refactored by extracting focused private helpers. No behavioural changes.Extractions
pkg/console/console.go—RenderTable(67 → ~46 lines)Pulled the
styleFuncclosure out intobuildTableStyleFunc(config, ttyCheck, dataRowCount).pkg/github/label_objective_mapping.go—ComputeObjectiveValue(67 → ~37 lines)Each
switchbranch moved to its own method:computeValueSum,computeValueFirst,computeValueMax.pkg/parser/schema_suggestions.go—findFieldLocationsInSchema(72 → ~14 lines)The two scanning loops extracted into
collectExactFieldMatchesandcollectFuzzyFieldMatches.