-
Notifications
You must be signed in to change notification settings - Fork 130
[BE-1996] Support .nvmrc version files for tool setup
#1233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ package versionfile | |||||
|
|
||||||
| import ( | ||||||
| "bufio" | ||||||
| "bytes" | ||||||
| "encoding/json" | ||||||
| "fmt" | ||||||
| "os" | ||||||
|
|
@@ -25,6 +26,8 @@ func Parse(path string) ([]ToolVersion, error) { | |||||
| return parseToolVersionsFile(path) | ||||||
| case ".fvmrc": | ||||||
| return parseFVMRC(path) | ||||||
| case ".nvmrc": | ||||||
| return parseNVMRC(path) | ||||||
| case "fvm_config.json": | ||||||
| return parseFVMConfigJSON(path) | ||||||
| default: | ||||||
|
|
@@ -45,6 +48,7 @@ func FindVersionFiles(dir string) ([]string, error) { | |||||
| ".tool-versions", | ||||||
| ".ruby-version", | ||||||
| ".node-version", | ||||||
| ".nvmrc", | ||||||
| ".python-version", | ||||||
| ".java-version", | ||||||
| ".go-version", | ||||||
|
|
@@ -157,6 +161,36 @@ func extractStringValue(config map[string]any, path string, key string) (string, | |||||
| return str, nil | ||||||
| } | ||||||
|
|
||||||
| // parseNVMRC parses an NVM .nvmrc file to extract the Node.js version. | ||||||
| func parseNVMRC(path string) ([]ToolVersion, error) { | ||||||
| content, err := os.ReadFile(path) | ||||||
| if err != nil { | ||||||
| return nil, fmt.Errorf("read %s: %w", path, err) | ||||||
| } | ||||||
|
|
||||||
| content = bytes.TrimSpace(content) | ||||||
|
|
||||||
| if len(content) == 0 { | ||||||
| return nil, fmt.Errorf("%s: empty version file", path) | ||||||
| } | ||||||
|
|
||||||
| for line := range strings.SplitSeq(string(content), "\n") { | ||||||
| line = strings.TrimSpace(line) | ||||||
| if line == "" || strings.HasPrefix(line, "#") || strings.Contains(line, "=") { | ||||||
| continue | ||||||
| } | ||||||
| version := strings.TrimPrefix(line, "v") | ||||||
marcell-vida marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| if version == "" { | ||||||
| return nil, fmt.Errorf("%s: invalid version (empty after removing 'v' prefix)", path) | ||||||
| } | ||||||
| return []ToolVersion{ | ||||||
| {ToolName: "node", Version: version}, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🐛 Bug The ToolName is hardcoded as "node", but the alias map in toolprovider/alias/alias.go shows that "node" is an alias for "nodejs". Other parsers like parseSingleToolVersion use inferToolID which calls GetCanonicalToolID to convert aliases to canonical tool IDs. This creates an inconsistency: .node-version files return "nodejs" (canonical), while .nvmrc files return "node" (alias). While the alias conversion happens later in run.go, the parser should be consistent with other parsers and return the canonical tool ID "nodejs". 🔄 Suggestion:
Suggested change
|
||||||
| }, nil | ||||||
| } | ||||||
|
|
||||||
| return nil, fmt.Errorf("%s: no valid version found", path) | ||||||
| } | ||||||
|
|
||||||
| // parseFVMRC parses an FVM 3.x .fvmrc JSON file to extract the Flutter version(s). | ||||||
| // Supports the main "flutter" key and optional "flavors" map. | ||||||
| func parseFVMRC(path string) ([]ToolVersion, error) { | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why not use
strings.TrimSpace()?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is faster than strings (minor difference). I know, a few milliseconds won't really count, but it was no effort to implement like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! It's a nitpick, I don't mind the current impl either!