refactor(detector): add context.Context to Manager.ListInstalled and Current#101
Merged
Merged
Conversation
…Current Mechanical refactor that adds context.Context as the first parameter to two Manager interface methods. The other interface methods (Install, Uninstall, Use, SetDefault, GlobalNpmPrefix, Version) are intentionally out of scope per the issue title. The omission became user-visible when PR #56 / issue #41 added Current() across all 8 manager implementations: a Ctrl-C mid-`nodeup upgrade` would still wait for the underlying `manager list` / `fnm list` / `nvm current` subprocess to finish before returning, because cancellation had no path into the read paths. Interface + 8 implementations (fnm, nvm, volta, asdf, mise, n, nodenv, nvm-windows) plumb ctx into existing runShell/runScript calls (both already ctx-aware via internal/platform/shell.go). 6 production call-sites in internal/cli/{check,list,upgrade,packages}.go pass cmd.Context(); getCurrentVersion(m) becomes getCurrentVersion(ctx, m). 3 mock/stub Manager types (fakeManager, listTestStubManager, stubManager) take a discarded _ context.Context. 39 test call-sites in the per-manager *_test.go files now pass t.Context() (Go 1.24). contextcheck linter is satisfied: production code uses cmd.Context(), tests use t.Context() (no context.Background() anywhere a ctx is in scope). Closes #53
|
✔️ 987aac6 - Conventional commits check succeeded. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Mechanical refactor that adds
context.Contextas the firstparameter to the
Managerinterface's two read methods:ListInstalledandCurrent. The other interface methods(
Install,Uninstall,Use,SetDefault,GlobalNpmPrefix,Version) are intentionally out of scope.Why
The
Managerinterface was first laid out withoutctxon anymethod. The omission became user-visible when the post-upgrade
cleanup feature (PR #56 / issue #41) added
Current()and shippedacross all 8 manager implementations: a Ctrl-C mid-
nodeup upgradewould still wait for the underlying
manager list/fnm list/nvm currentsubprocess to finish before returning, becausecancellation had no path into the read paths.
Install/Uninstallalready had a cancellation story (the platform lock +interrupted-upgrade sentinel from #46), so they aren't part of
this PR.
What
internal/detector/detector.go— interface signatures:ListInstalled(ctx context.Context) ([]semver.Version, error)Current(ctx context.Context) (semver.Version, error)cmd.Context()as the source of
ctx.internal/detector/*.go:fnm.go,nvm.go,volta.go,asdf.go,mise.go,n.go,nodenv.go,nvm_windows.go. Each method signature updated;each
runShell(context.Background(), ...)/runScript(context.Background(), ...)inside the bodiesreplaced with the new
ctxparameter.nvm_windows.go'sCurrentis the lone non-shell body (returns theErrNVMWindowsNotImplementedsentinel) — it just gets adiscarded
_ context.Contextparameter.internal/cli/{check,list, upgrade,packages}.gonow passcmd.Context().packages.go'shelper
getCurrentVersion(m)becomesgetCurrentVersion(ctx, m); its sole caller at line 50 passescmd.Context().*_test.go:fakeManager(system_node_test.go),listTestStubManager(cli/list_test.go),
stubManager(cli/cleanup_test.go). Eachtakes a discarded
_ context.ContextonListInstalledandCurrent; bodies unchanged.*_test.gofiles nowpass
t.Context()(Go 1.24 addedt.Context(); the project'sgo.modis ongo 1.24).Linter verification
contextcheckis enabled in.golangci.yml:48and (per theexploration) the
_test\.goexclusion only coverserrcheck/gocritic, notcontextcheck. So production call-sites with acobra
cmd.Context()in scope must use it — notcontext.Background(). All 6 production call-sites do so.make lintis green.Linked issues
Closes #53
Type of change
refactor— no behavior changefeat— new feature (MINOR bump)fix— bug fix (PATCH bump)perf— performance improvementtest— tests onlydocs— documentation onlychore— maintenance / dependency bumpci— CI/CD onlybuild— build system onlyChecklist
refactor(scope): subject)make cilocally and it passes (go vet,golangci-lint run(includingcontextcheck),go test -race -coverprofile=coverage.out ./...all green; coverage59.6% unchanged)
39 existing test call-sites updated to the new signature, no
behavior change)
CHANGELOG.mdgets a### Changedentry under[Unreleased])make lintgreen)body and updated CHANGELOG.md
Files touched (24)
Interface + 8 managers (10 files in
internal/detector/):detector.go,fnm.go,nvm.go,volta.go,asdf.go,mise.go,n.go,nodenv.go,nvm_windows.go, and theproduction test stub
system_node_test.go.Production call-sites (4 files in
internal/cli/):check.go,list.go,upgrade.go,packages.go.Test stubs + call-sites (8 files in
*_test.go):cli/list_test.go,cli/cleanup_test.go, and the 6 per-managertest files in
internal/detector/:asdf_test.go,fnm_test.go,mise_test.go,n_test.go,nodenv_test.go,nvm_test.go,nvm_windows_test.go,volta_test.go(one of those is the 8th —the stubManager in
cli/cleanup_test.gois the 9th).Changelog:
CHANGELOG.md.