[typist] π€ Typist β Go Type Consistency Analysis (pkg/) #41686
Replies: 1 comment 1 reply
-
|
/q update to create issue with agent tasks, rather than inlining in body. |
Beta Was this translation helpful? Give feedback.
1 reply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
-
π€ Typist β Go Type Consistency Analysis
Analysis of repository: github/gh-aw
Executive Summary
Good news first: gh-aw is already a remarkably type-disciplined codebase. I scanned all 959 non-test
.gofiles underpkg/(~700+ exported type definitions across ~30 packages) hunting for duplicated types and weak typing β and most of what a naive name-matcher flags as duplicates turns out to be deliberate, well-documented type aliases. For examplepkg/workflowexposesActionPin,InputDefinition, andSanitizeOptions, but every one of them istype X = otherpkg.Xβ a single source of truth re-exported for ergonomic local use. That is exactly the consolidation pattern this kind of review usually recommends, already in place. πSo this is less a you-have-a-mess report and more a here-are-the-remaining-rough-edges one. The genuine opportunities are narrow: one semantic-duplicate cluster where the GitHub pull request concept is modeled three independent ways, and a pervasive β but mostly idiomatic β use of
map[string]anyfor YAML/JSON frontmatter parsing where a handful of hot paths would benefit from typed structs. Rawinterface{}is effectively extinct (16 occurrences total, almost all in tests/testdata), and constants are already wrapped in semantic types (type WorkflowID string,type LineLength int, ...). Net: the codebase is in great shape; the suggestions below are polish, not triage.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
type X = pkg.Xaliases_wasm.go): intentional, excludedCluster 1: shared-type collisions β β already consolidated (no action)
The name-level collisions all resolve to aliases backed by a single struct. These are a strength, not a problem β listed so reviewers do not re-fix what is already correct:
workflow.ActionPin,ActionYAMLInput,ContainerPin,ActionPinsData,SHAResolveractionpins.*pkg/workflow/action_pins.go:18-31workflow.InputDefinitiontypes.InputDefinitionpkg/workflow/inputs.go:18workflow.SanitizeOptionsstringutil.SanitizeOptionspkg/workflow/strings.go:94workflow.CloseIssuesConfig,ClosePullRequestsConfig,CloseDiscussionsConfigCloseEntityConfigpkg/workflow/close_entity_helpers.go:203-205workflow.MissingDataConfig,MissingToolConfig,ReportIncompleteConfigIssueReportingConfigpkg/workflow/missing_issue_reporting.go:26-42close_entity_helpers.goeven documents why close-entity uses one sharedCloseEntityConfig+ registry while update-entity intentionally splits per type. That kind of reasoning makes this codebase easy to trust.Cluster 2: Pull-request models β semantic duplicate (Medium)
Type: Semantic duplicate Β· Occurrences: 3 Β· Impact: Medium β one domain concept modeled three independent ways
The GitHub pull request resource is represented by three unrelated structs in three packages, each carrying a different partial field set:
Recommendation: These serve genuinely different consumers, so a forced merge would be wrong. But the shared core (Number, Title, identity) is re-declared each time. Consider a small shared
types.PullRequestRefwith Number/Title/URL, embedded by each, leaving specialized fields local. Low urgency.Excluded (intentional, by design)
SpinnerWrapper(spinner.go+spinner_wasm.go),ProgressBar(progress.go+progress_wasm.go),RepositoryFeatures(repository_features_validation.go+_wasm.go) β build-tag variants of the same type. Correct as-is.Untyped Usages
Summary Statistics
interface{}(legacy spelling): 16 total, ~all in*_test.go/testdata/+ 1 inpkg/workflow/compiler_types.goβ effectively eliminatedmap[string]any: pervasive (hundreds of sites acrosspkg/cli,pkg/workflow,pkg/parser) β overwhelmingly idiomatic frontmatter/JSON parsingpkg/constantsalready wraps primitives in named typesCategory 1:
map[string]anyfor frontmatter β mostly appropriate (Low/selective)The bulk of
anyusage is parsing untyped YAML frontmatter and JSON intomap[string]any, then asserting per-key. For an intentionally open/forward-compatible schema this is the right Go idiom, and the repo already centralizes conversions inpkg/typeutil/convert.goandpkg/parsermergers β a good mitigation.Where typing would pay off: hot paths that repeatedly assert the same shape out of a
map[string]any. Thecodemod_*andsafe_outputs_parserfamilies re-extract well-known fields (engine, permissions, safe-outputs.*) by key + assertion. Promoting the most-accessed of those to typed structs (parse once at the boundary) removes scattered per-site assertions.Category 2: Untyped constants β β already strong (no action)
Counter to the usual finding, gh-aw constants are already semantically typed. Representative examples from
pkg/constants:No magic timeout literals begging for a
type Seconds intwere found in the scanned set. Keep doing this.Refactoring Recommendations (prioritized)
Priority 1 β Low effort, optional: unify PR identity
types.PullRequestRefwith Number/Title/URL.cli.PullRequest,cli.PRInfo,intent.PullRequestData; keep specialized fields local.go build ./...+ tests. ~1β2h, Medium clarity benefit.Priority 2 β Selective: type the hottest frontmatter fields
codemod_*/safe_outputs_parser.go.map[string]anyfor the genuinely open parts.Priority 3 β Hygiene: retire the last raw
interface{}interface{}inpkg/workflow/compiler_types.gowithanyfor consistency. ~5 min.Implementation Checklist
types.PullRequestRefand embed in the 3 PR structsinterface{}βanygo build ./...+ full test suite_wasm.govariants untouchedAnalysis Metadata
interface{}(non-test): 1Beta Was this translation helpful? Give feedback.
All reactions