refactor(ixgo): use aggregate package registration#158
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
FennoAI Review — PR #158: refactor(ixgo): use aggregate package registration
Collapsing ~150 hand-maintained blank imports in internal/ixgo/pkgs.go into the single aggregate _ "github.com/goplus/ixgo/pkg" is a real maintainability win, and I verified the mechanics hold up locally:
go build ./...→ exit 0go mod tidy→ no changes (go.mod/go.sum are tidy-clean)go mod verify→ all modules verified- Keeping
net/rpc/net/rpc/jsonrpcseparate is correct — the aggregate does not register them (confirmed againstgoplus/ixgo@v1.1.1/pkg/go124_pkgs.go). - The
testing/*exclusion is preserved — the aggregate does not includetesting/*for go1.24. - The 8
//go:generate qexpdirectives still map 1:1 to the local generated imports, so they are not stale.
The one substantive concern is a behavioral change hidden inside a "refactor", called out inline below. A few non-line-specific notes:
Implicit testing/* exclusion is now coincidental, not enforced. Previously the excluded packages were visible in the file. Under the aggregate, testing/* stays out only because the current upstream aggregate happens to omit it; a future ixgo bump could silently register it. Consider a small test that pins the expected registered set (asserts net/rpc* present, testing absent) so these invariants survive future dependency bumps.
Binary size / linker weight. The aggregate force-links every registered subpackage (160 for go1.24), including the newly-added debug/* readers (debug/dwarf, debug/elf, debug/macho, …) and their transitive deps. The runtime/startup delta is negligible for a CLI, but binary size will grow. A quick before/after ls -l on the built binary would confirm the increase is acceptable. (os/exec and syscall were already registered before this PR — pre-existing, not introduced here.)
Overall: the refactor is fine to land; please just confirm the debug/* re-inclusion is intended (and ideally reframe as more than a pure refactor, since it changes what interpreted code can reach).
81878b2 to
571ad59
Compare
No description provided.