feat(tool): avoid importing every manifest to resolve tools.<name>.latest#498
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds a ChangesPer-tool index.nix generation and consumption
Sequence DiagramsequenceDiagram
participant generate.go
participant writeIndex
participant fs as Manifest directory
participant template as text/template
generate.go->>writeIndex: writeIndex(outputDir)
writeIndex->>fs: read *.nix files (exclude index.nix)
fs-->>writeIndex: manifest file list
writeIndex->>fs: read each manifest, extract go = "..." via regex
writeIndex->>writeIndex: semver-sort entries descending
writeIndex->>template: execute index.nix template
template-->>writeIndex: rendered Nix attrset
writeIndex->>fs: write index.nix (mode 0644)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mk-tool-set.nix`:
- Around line 61-62: The code assumes every version in `toolData.sortedVersions`
exists as a key in `toolData.index`, but if `index.nix` is stale or incomplete,
accessing `toolData.index.${v}.go` will fail with an attribute-missing error.
Guard the lookups by checking that the attribute exists in `toolData.index`
before accessing it, or use Nix's optional attribute access pattern (such as
`toolData.index.${v}` with a conditional check or hasAttr verification) to
safely skip versions that are not present in the index. Apply this guard to both
the map operation shown in the diff and the similar pattern occurring at the
other affected location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4bb5997-7bbf-48b2-8587-c0cd87a5f733
📒 Files selected for processing (13)
.github/workflows/tools-update.ymlinternal/cli/goscrape/modproxy/generate.gointernal/cli/goscrape/modproxy/index.gointernal/cli/goscrape/modproxy/index_test.gointernal/cli/goscrape/modproxy/testdata/index.nix.goldenlib/mk-tool-set.nixlib/tool-manifests.nixmanifests/delve/index.nixmanifests/gofumpt/index.nixmanifests/golangci-lint/index.nixmanifests/gopls/index.nixmanifests/govulncheck/index.nixmanifests/staticcheck/index.nix
…test Closes #492 `lib/mk-tool-set.nix` computed `compatibleVersions`/`latestCompatible` by filtering over every manifest in a tool's directory, forcing Nix to parse every <version>.nix file just to read its `go` requirement. For golangci-lint (95 manifests, 5.6MB), this added ~54ms to every eval of tools.golangci-lint.latest (442ms -> 389ms once fixed). Add a per-tool manifests/<tool>/index.nix mapping version -> { go }, loaded by lib/tool-manifests.nix and consumed by lib/mk-tool-set.nix instead of the full manifests. goscrape's mod-proxy generate now (re)writes index.nix by scanning the go requirement out of every manifest in the output directory, sorted newest-first via golang.org/x/mod/semver, and tools-update.yml stages it alongside the generated manifest. Backfilled index.nix for all 6 existing tool directories. Signed-off-by: purpleclay <purpleclaygh@gmail.com>
9e30453 to
68db957
Compare
closes #492
Summary by CodeRabbit
index.nixcompatibility maps for Delve, gofumpt, golangci-lint, gopls, govulncheck, and staticcheck.index.nixgeneration via a golden-file check.