fix(idiomatic): use generic parser for idiomatic files#8171
Conversation
Summary of ChangesHello @risu729, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the handling of idiomatic version files to introduce a more robust and centralized parsing mechanism. The primary goal is to ensure consistent and correct interpretation of version specifications, particularly from Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the parsing of idiomatic version files to use a more generic approach, with special handling for package.json and a fallback to raw text parsing. The changes are well-structured and consistently applied across the codebase. I've identified a few areas for improvement related to performance and code clarity. Specifically, I've suggested refactoring the parsing logic to avoid redundant file reads and parsing, and a small change to make some of the new code more idiomatic.
There was a problem hiding this comment.
Pull request overview
This PR refactors idiomatic file parsing to address an issue where vfox and other plugins that don't support package.json parsing experienced unexpected behavior. The change creates a generic parsing system with a defined precedence order: package.json (built-in), plugin-specific parsing, and raw text file fallback.
Changes:
- Centralized
package.jsonparsing logic into a dedicated module - Changed
parse_idiomatic_fileto returnVec<String>instead ofStringto support multiple versions - Renamed
idiomatic_filenamesto_idiomatic_filenamesin backend implementations to distinguish backend-specific filenames from the combined list
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/backend/mod.rs | Implements new _idiomatic_filenames and refactored parse_idiomatic_file signature |
| src/backend/asdf.rs | Updates to return Vec<String> from parse_idiomatic_file and renames idiomatic_filenames |
| src/backend/vfox.rs | Updates to return Vec<String> from parse_idiomatic_file and renames idiomatic_filenames |
| src/config/config_file/idiomatic_version.rs | Implements parsing precedence logic with built-in package.json parser |
| src/config/config_file/mod.rs | Simplifies idiomatic filename check logic |
| src/config/mod.rs | Renames function to load_idiomatic_filenames and enables idiomatic file support in config hierarchy loading |
| src/package_json.rs | Moves from private module to public module with new parse function that centralizes tool-specific logic |
| src/plugins/core/*.rs | Updates all core plugins to use new _idiomatic_filenames and parse_idiomatic_file signatures |
| src/main.rs | Removes package_json module declaration as it's now in idiomatic_version module |
| docs/configuration.md | Updates documentation with newly supported idiomatic files for various tools |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdx
left a comment
jdx
left a comment
There was a problem hiding this comment.
Code Review (AI-generated)
The core idea of centralizing idiomatic file parsing (especially package.json) is solid — nice deduplication across bun, deno, and node backends.
Issues
1. Error propagation change — behavioral regression
The old code logged a trace when parse_idiomatic_file failed and continued to the next plugin. The new code propagates with ?:
let versions = plugin.parse_idiomatic_file(&path).await?;This means a plugin returning an error will now abort parsing for all remaining plugins for that file, instead of skipping the failed one and trying others. For example, if a vfox plugin fails to parse .node-version, the core node plugin won't get a chance.
2. from_file now filters by enable_tools — duplicating existing filtering
The new enable_tools check in from_file:
let enable_tools = crate::config::Settings::get()
.idiomatic_version_file_enable_tools
.clone();
for b in backend::list().into_iter() {
if !enable_tools.contains(b.id()) {
continue;
}This filtering already happens upstream in load_all_config_files. If from_file is also called from other paths this could unexpectedly restrict behavior. If it's intentional for safety, a comment would help.
3. Hardcoded tool name matching in package_json::parse
match tool_name {
"node" | "deno" => pkg.runtime_version(tool_name),
"bun" => pkg.runtime_version(tool_name)
.or_else(|| pkg.package_manager_version(tool_name)),
"npm" | "yarn" | "pnpm" => pkg.package_manager_version(tool_name),
_ => None,
};If someone adds a new tool that uses package.json, they need to modify this match arm too. A comment explaining this is intentional would help.
4. Node parser may return empty string version
In node.rs, if the file is empty or only a comment, body will be "" and it returns vec![""]. The outer code only checks !versions.is_empty(), so an empty string passes through as a "version". The old code masked this via split_whitespace() in the caller. Now with the new pipeline, the raw text fallback is only used when the plugin returns an empty vec, but the node plugin always returns a vec with one element.
5. Duplicate filenames possible
idiomatic_filenames() merges _idiomatic_filenames() with registry entries:
let mut filenames = self._idiomatic_filenames().await?;
if let Some(rt) = REGISTRY.get(self.id()) {
filenames.extend(rt.idiomatic_files.iter().map(|s| s.to_string()));
}If a backend already returns "package.json" from _idiomatic_filenames() and the registry also has it, the vec will contain duplicates. A .dedup() or BTreeSet would be cleaner.
|
Thanks! Fixed the reviewed points. |
There was a problem hiding this comment.
Note
This review was generated by AI (Claude).
Good fix for the core issue — centralizing package.json parsing so vfox plugins don't need to implement it themselves is the right approach. A few things to address:
Bug: raw text fallback triggers when parse_idiomatic_file returns Ok(vec![])
The new fallback logic in idiomatic_version/mod.rs reads the raw file content whenever parse_idiomatic_file returns an empty vec. But backends that implement a custom parser (like node) also return Ok(vec![]) to signal "no version found" — not "use fallback".
Concrete regression: a .nvmrc containing only a comment like # lts will now produce broken versions:
- Node's
parse_idiomatic_filestrips comments → returnsOk(vec![]) - Fallback reads raw text
# ltsand callsadd_versionwith"#"and"lts"as version strings
vfox behavior change: previously vfox::parse_idiomatic_file returned Err when no version was found (triggering continue/skip). Now it returns Ok(vec![]), which triggers the raw text fallback — the opposite of the intended fix.
The cleanest fix is to move the raw text fallback into the default parse_idiomatic_file implementation (as it was before), and remove it from IdiomaticVersionFile::parse:
// In Backend trait default impl
async fn parse_idiomatic_file(&self, path: &Path) -> eyre::Result<Vec<String>> {
let contents = file::read_to_string(path)?;
let trimmed = contents.trim();
if trimmed.is_empty() {
return Ok(vec![]);
}
Ok(trimmed.split_whitespace().map(|s| s.to_string()).collect())
}Then IdiomaticVersionFile::parse just uses whatever the backend returns without a separate fallback. Backends with custom parsing override and never fall through.
Lost debug logging in config_file/mod.rs
// Before
Err(e) => {
debug!("idiomatic_filenames failed for {}: {:?}", b, e);
}
// After (errors silently dropped)
if let Ok(filenames) = b.idiomatic_filenames().await
&& filenames.contains(&file_name)Worth keeping the debug log. Could be written as:
match b.idiomatic_filenames().await {
Ok(filenames) if filenames.contains(&file_name) => return true,
Err(e) => debug!("idiomatic_filenames failed for {}: {:?}", b, e),
_ => {}
}Minor: typo in package_json.rs
// "pacakge" → "package"
// We ignore unknown tools in pacakge.jsonThe overall structure and the package.json centralization are solid. The test for MockBackend is a nice addition. The bonus fix for load_config_files_from_paths (was a TODO) is appreciated.
|
Sorry, I misunderstood the code flow, fixed now. |
jdx
left a comment
jdx
left a comment
There was a problem hiding this comment.
Code Review (AI-generated)
The core idea of centralizing package.json parsing so vfox/asdf plugins don't need to handle it themselves is solid — nice deduplication across bun, deno, and node backends.
Issues
1. Error-to-fallback semantic change
In idiomatic_version/mod.rs, when parse_idiomatic_file returns Err, the new code falls back to raw text parsing. The old code just skipped the plugin:
// Old behavior
Err(e) => {
trace!("skipping {} for {}: {e}", plugin.id(), path.display());
continue;
}
// New behavior
Err(_) => {
// Falls through to raw text parsing
let body = crate::file::read_to_string(&path).unwrap_or_default();
...
}The default trait impl eyre::bail!("no custom parser") intentionally triggers this fallback, which is fine. But real plugin errors (e.g., vfox network failure, malformed plugin) will also hit it, producing garbage versions from raw text instead of being skipped.
The cleanest fix is to move the raw text fallback into the default parse_idiomatic_file impl:
async fn parse_idiomatic_file(&self, path: &Path) -> eyre::Result<Vec<String>> {
let contents = file::read_to_string(path)?;
let trimmed = contents.trim();
if trimmed.is_empty() {
return Ok(vec![]);
}
Ok(trimmed.split_whitespace().map(|s| s.to_string()).collect())
}Then IdiomaticVersionFile::parse just uses whatever the backend returns without a separate fallback, and backends with custom parsers override normally.
2. Hardcoded tool list in package_json::parse
The match on tool names means adding a new tool that uses package.json requires updating this central match arm — easy to miss:
match tool_name {
"node" | "deno" => pkg.runtime_version(tool_name),
"bun" => pkg.runtime_version(tool_name)
.or_else(|| pkg.package_manager_version(tool_name)),
"npm" | "yarn" | "pnpm" => pkg.package_manager_version(tool_name),
_ => None,
};This should be dispatched to the backends. One approach — add a trait method declaring the extraction strategy:
enum PackageJsonField {
Runtime,
PackageManager,
RuntimeThenPackageManager,
}
// Backend trait — default: no package.json support
fn package_json_field(&self) -> Option<PackageJsonField> {
None
}Each backend declares its own strategy (node/deno → Runtime, bun → RuntimeThenPackageManager, npm/yarn/pnpm → PackageManager), and the central parser dispatches on the enum instead of tool name strings. New tools just implement package_json_field().
3. Lost trace logging on parse failures
The old code had trace!("skipping {} for {}: {e}", ...) when a plugin failed to parse. The new fallback path is silent. Worth adding a trace log.
Minor
- The
load_config_files_from_pathschange (removing the TODO, now loading idiomatic filenames for monorepo contexts) is a nice bonus fix but is a behavioral change worth calling out in the PR description.
What looks good
Vec<String>return type correctly models multiple versions- New tests (MockBackend, package_json parse, engines-ignored) are solid
unique()call inidiomatic_filenames()handles dedup- Docs table update is thorough
config_file/mod.rsmatch simplification retains debug logging
|
What do you think of adding npm, yarn, and pnpm as core tools? I'm happy with continuing this PR, but I just want to confirm before. |
|
ah yeah I forgot they weren't core tools, I guess we can just keep it hardcoded then |
It was intended. I intended to support adding idiomatic files like However, the review is correct, so what if we only accept version-like strings, maybe checked by regex? |
|
The error-to-fallback change is a regression. Previously when Err(e) => {
trace!("skipping {} for {}: {e}", plugin.id(), path.display());
continue;
}Now The fix suggested in the reviews is the right one: move the raw text fallback into the default |
risu729
force-pushed
the
fix-idiomatic
branch
from
b1eab92 to
797afa7
Compare
|
Oh, that's true. Sorry I didn't realize that. |
jdx
left a comment
jdx
left a comment
There was a problem hiding this comment.
Code Review
Note: This review was AI-generated using Claude Code.
Summary
Centralizes package.json parsing so all tools (including vfox plugins) get consistent behavior. Previously bun, deno, and node each duplicated their own package.json handling, and vfox backends couldn't parse it at all. Changes parse_idiomatic_file to return Vec<String>, moves package.json parsing into a dedicated module, and introduces _idiomatic_filenames() as the backend-specific override with idiomatic_filenames() merging backend + registry results.
Issues
package.json special-casing is in the wrong layer (medium)
In IdiomaticVersionFile::parse(), there's a hardcoded if path.file_name() == "package.json" that bypasses plugin parsing entirely. The old code had this check in the Backend trait's default parse_idiomatic_file — that was a better location because it meant any caller of parse_idiomatic_file got correct package.json handling automatically. With the new code, if anything calls backend.parse_idiomatic_file() directly with a package.json path, it falls through to the raw text parser instead of the JSON parser.
Suggest moving the package.json check back into the default parse_idiomatic_file in Backend.
Positive aspects
- Good deduplication —
package.jsonparsing was copied in 3+ places - Clean test coverage with
MockBackendandpackage_jsonunit tests Vec<String>return type is more correct since files like.python-versionscan list multiple versions- Documentation table update is thorough
risu729
force-pushed
the
fix-idiomatic
branch
from
12b90ea to
ec66033
Compare
|
Sorry for taking long. I fixed the reviewed issue. |
|
I think this is probably a real issue: In ruby.rs and ruby_windows.rs, when parse_gemfile returns an empty string (no match), this code wraps it: Same issue in java.rs line Ok(vec![contents.trim().to_string()]) — if contents is empty after trimming. |
|
Sorry, that's so true. I fixed the bug |
|
sorry @risu729 this needs a rebase |
This PR fixes jdx#7379 (comment), where unexpected behavior occurred (e.g., yarn with package.json) because some plugins (like vfox) do not support package.json parsing. This change genericizes the parsing of built-in supported idiomatic files (currently and raw text files). If a file is declared as an idiomatic filename for a tool, the built-in parser will be used. The precedence order is: 1. (built-in parser) 2. Plugin-specific parsing () 3. Raw text file (built-in fallback) Note: Plugins cannot override the parsing logic for if it is handled by the built-in parser. This means specific tools cannot have custom parsing logic that differs from the core implementation (e.g., ignoring field but respecting for a specific tool is not possible via plugin override).
This reverts commit 4910b7a.
…ted or threw an error
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a solid refactoring that genericizes and centralizes the parsing of idiomatic version files. The introduction of a new pattern for backend traits with public and overridable methods, along with special handling for package.json, is a significant improvement. I've identified a couple of potential regressions: the default implementation for parsing idiomatic files no longer handles comments, and the Java plugin's parsing might not correctly handle multiple versions on a single line. I've included specific suggestions to address these points. Overall, this is a well-executed refactoring that enhances the codebase's structure and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed generic parser for idiomatic version files, which is a great improvement. It successfully centralizes package.json parsing and provides a sensible default for other text-based version files, reducing code duplication across plugins. The use of _ prefixed methods for backend-specific implementations is a clean and effective pattern. I have one suggestion regarding a potential performance optimization to avoid redundant work when identifying and parsing these files.
| pub async fn from_file(path: &Path) -> Result<Self> { | ||
| trace!("parsing idiomatic version: {}", path.display()); | ||
| let file_name = path.file_name().unwrap().to_string_lossy().to_string(); | ||
| let mut tools: Vec<Arc<dyn Backend>> = vec![]; | ||
| for b in backend::list().into_iter() { | ||
| if b.idiomatic_filenames() | ||
| .await | ||
| .is_ok_and(|f| f.contains(&file_name)) | ||
| { | ||
| tools.push(b); | ||
| } | ||
| } | ||
| Self::parse(path.to_path_buf(), tools).await | ||
| } |
There was a problem hiding this comment.
This function iterates through all backends to find which ones are associated with the given idiomatic file. This is inefficient because a similar iteration already happens in detect_config_file_type (via filename_is_idiomatic) to identify this as an idiomatic version file in the first place.
To avoid this double work, I suggest a refactoring:
- Modify
filename_is_idiomaticto return the list of matching backend plugins instead of just a boolean. - Pass this list of plugins through
detect_config_file_typetoconfig_file::parse. - In
config_file::parse, callIdiomaticVersionFile::parsedirectly with the list of plugins, which would remove the need for thisfrom_filefunction.
This would centralize the logic and improve performance by iterating through backends only once.
There was a problem hiding this comment.
True, but I'm gonna work on this in a separate PR.
Greptile SummaryThis PR refactors idiomatic file parsing to use a generic parser that handles Key Changes:
Architecture Impact: Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start[IdiomaticVersionFile.from_file] --> GetFilename[Get filename from path]
GetFilename --> FindBackends[Find all backends with this filename in idiomatic_filenames]
FindBackends --> Loop{For each backend}
Loop --> CallParse[Call backend.parse_idiomatic_file]
CallParse --> IsPackageJson{Is filename == package.json?}
IsPackageJson -->|Yes| BuiltInParser[Use built-in package_json::parse]
IsPackageJson -->|No| CallPrivate[Call backend._parse_idiomatic_file]
BuiltInParser --> ParsePkgJson[Parse devEngines & packageManager fields]
ParsePkgJson --> SimplifySemver[Simplify semver ranges]
SimplifySemver --> ReturnVersions[Return Vec<String>]
CallPrivate --> HasOverride{Backend has _parse_idiomatic_file override?}
HasOverride -->|Yes| CustomParser[Use custom parser logic]
HasOverride -->|No| DefaultParser[Use default: normalize_idiomatic_contents then split_whitespace]
CustomParser --> ReturnVersions
DefaultParser --> ReturnVersions
ReturnVersions --> CheckError{Returned Error?}
CheckError -->|Yes| Skip[Skip this backend, log trace]
CheckError -->|No| AddVersions[Add versions to ToolRequestSet]
AddVersions --> Loop
Skip --> Loop
Loop -->|Done| Return[Return IdiomaticVersionFile with all collected versions]
Last reviewed commit: e37b8a1 |
### 🚀 Features - **(hooks)** add task references to hooks and watch_files by @jdx in [#8400](#8400) - **(prepare)** add git-submodule built-in provider by @jdx in [#8407](#8407) - **(prepare)** add human-readable stale reasons to prepare output by @jdx in [#8408](#8408) - **(prepare)** add dependency ordering to prepare steps by @jdx in [#8401](#8401) - **(prepare)** add --explain flag for provider diagnostics by @jdx in [#8409](#8409) - **(prepare)** add per-provider timeout support by @jdx in [#8405](#8405) - **(prepare)** add blake3 content-hash freshness checking by @jdx in [#8404](#8404) - **(tasks)** monorepo vars and per-task vars by @halms in [#8248](#8248) ### 🐛 Bug Fixes - **(aqua)** restore bin_paths disk cache with fresh_file invalidation by @jdx in [#8398](#8398) - **(idiomatic)** use generic parser for idiomatic files by @risu729 in [#8171](#8171) - **(install)** apply precompiled options to all platforms in lockfile by @jdx in [#8396](#8396) - **(install)** normalize "v" prefix when matching lockfile versions by @jdx in [#8413](#8413) - **(prepare)** improve git submodule parser and fix check_staleness error handling by @jdx in [#8412](#8412) - **(python)** respect precompiled settings in lock file generation by @jdx in [#8399](#8399) - **(python)** clarify uv_venv_auto docs + prevent uv shim recursion in venv creation by @halms in [#8402](#8402) - **(task)** remove deprecated `# mise` task header syntax by @jdx in [#8403](#8403) - **(vfox)** avoid eager metadata loading during config file detection by @jdx in [#8397](#8397) - clarify GitHub attestations to be artifact ones by @scop in [#8394](#8394) - ignore comments in idiomatic version files by @iloveitaly in [#7682](#7682) ### 🚜 Refactor - unify archive detection by @risu729 in [#8137](#8137) ### 📚 Documentation - remove duplicated docs for npm.package_manager by @risu729 in [#8414](#8414)
This PR fixes #7379 (comment), where unexpected behavior occurred (e.g., yarn with package.json) because some plugins (like vfox) do not support package.json parsing.
This change genericizes the parsing of built-in supported idiomatic files (currently
package.jsonand raw text files). If a file is declared as an idiomatic filename for a tool, the built-in parser will be used.The precedence order is:
package.json(built-in parser)parse_idiomatic_file)Note: Plugins cannot override the parsing logic for
package.jsonif it is handled by the built-in parser. This means specific tools cannot have custompackage.jsonparsing logic that differs from the core implementation (e.g., ignoringpackageManagerfield but respectingdevEnginesfor a specific tool is not possible via plugin override).