fix(ixgo): export comparator classfile package - #156
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(ixgo): export comparator classfile package
No blocking issues. The change correctly registers github.com/goplus/llar/cmp with the ixgo interpreter so .gox comparators loaded outside a Go module can resolve their imports from the in-memory registry instead of falling back to on-disk (go/build) resolution, which fails without a module context. The new TestLoadComparator_OutsideModule exercises exactly this path via t.Chdir(t.TempDir()), and I verified the assertion matches the semver-based CJSON_cmp.gox testdata.
Verified:
- Generated
export.gomatches the existingformula/export.gopattern and declares all exported symbols ofcmp(CmpApp,Gopt_CmpApp_Main,GopPackage).Depscorrectly lists onlymod/module(the sole import ofcmp/cmp.go). pkgs.goblank import is alphabetized within the llar group;//go:generatedirective placed beside the siblingformuladirective.go build ./internal/ixgo/...succeeds.filepath.Abs(...)is captured beforet.Chdir, so the path resolution is correct.
Not verified: go test ./internal/modules/ could not run to completion in the sandbox due to a pre-existing ixgo link error (math/rand/v2.globalRand) unrelated to this PR.
The inline comments below are minor, optional polish only.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
5747471 to
d5679c4
Compare
There was a problem hiding this comment.
Review: fix(ixgo): export comparator classfile package
Non-blocking review. The change is well-scoped and correct: the generated export.go registers github.com/goplus/llar/cmp with the ixgo interpreter and is structurally consistent with the sibling formula/export.go. I built ./internal/ixgo/... and ./cmp/... (passes) and ran go test ./internal/modules/ -run TestLoadComparator (all pass, including the new TestLoadComparator_OutsideModule). No security or performance concerns from this diff — the registration runs once in init(), and the comparator loader is memoized via sync.OnceValues.
The findings below are maintainability/coverage/accuracy nits, not blockers. Note that #2–#4 were raised in the prior FennoAI review on this PR and appear to still be unaddressed.
Highest-value finding — new Libpng_cmp.gox is not exercised by any test. The PR's stated point is to validate that the lowercase semver.compare mapping works through ixgo, but no comparator-loading test reads the testdata/kodo-e2e/formulas tree. The real load path (loadOrDefaultComparator globbing *_cmp.gox) is only driven from modules.Load, and the internal/build e2e tests load from internal/build/testdata/formulas (see setupTestStore / testFormulaDir), not the kodo-e2e dir. The kodo-e2e main.go harness and the internal/build/http tests that reference the kodo-e2e root never invoke the comparator path. So this fixture — and the lowercase form specifically — has no CI coverage. Consider adding a unit test that loads it through loadComparatorFS (analogous to TestLoadComparator_WithRealTestData), or folding the lowercase form into an already-exercised comparator.
Minor: in internal/ixgo/pkgs.go the new cmp //go:generate directive is placed after formula, while the blank-import block below is alphabetized (cmp before formula). Cosmetic, but placing cmp before formula in the generate list too keeps the file self-consistent.
Inline comments below cover the remaining points.
2 participants
No description provided.