fix(config): preprocess paths per config file#1036
fix(config): preprocess paths per config file#1036lewis6991 wants to merge 1 commit intoEmmyLuaLs:mainfrom
Conversation
![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 configuration loading mechanism to ensure relative paths in settings like library and packages are resolved against their originating file's directory before merging. It introduces load_config_json_unprocessed for raw access and a new load_configs for runtime use, supported by a new path normalization utility. A review comment highlights a potential bug where a failed configuration parse could lead to merging default values and overwriting valid settings, suggesting that invalid files should be skipped instead.
lewis6991
force-pushed
the
split/config-loading-paths
branch
from
58aefdd to
3712627
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration loading logic to support per-file path preprocessing and normalization, ensuring relative paths in library and package settings are resolved correctly based on the configuration file's location. The review identified a critical bug where converting configurations to the Emmyrc struct before merging causes default values to overwrite settings from earlier files. Additionally, partial configurations from the client are no longer being flattened, which may cause settings with dotted keys to be ignored. A redundant preprocessing call in the new unit tests was also noted for removal.
| let mut emmyrc: Emmyrc = match serde_json::from_value(config_emmyrc_json) { | ||
| Ok(emmyrc) => emmyrc, | ||
| Err(err) => { | ||
| log::error!( | ||
| "Failed to read config file: {:?}, error: File not found or unreadable", | ||
| config_file | ||
| "Failed to parse config file as emmyrc {:?}: {:?}", | ||
| config_file, | ||
| err | ||
| ); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| let config_value = if config_file.extension().and_then(|s| s.to_str()) == Some("lua") { | ||
| match load_lua_config(&config_content) { | ||
| Ok(value) => value, | ||
| Err(e) => { | ||
| log::error!( | ||
| "Failed to parse lua config file: {:?}, error: {:?}", | ||
| &config_file, | ||
| e | ||
| ); | ||
| continue; | ||
| if let Some(config_root) = config_file.parent() { | ||
| emmyrc.pre_process_emmyrc(config_root); | ||
| } | ||
|
|
||
| match serde_json::to_value(emmyrc) { | ||
| Ok(config_emmyrc_json) => { | ||
| if has_config { | ||
| merge_values(&mut merged_emmyrc_json, config_emmyrc_json); | ||
| } else { | ||
| merged_emmyrc_json = config_emmyrc_json; | ||
| has_config = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Converting the config to the Emmyrc struct and then back to JSON for merging introduces a critical bug: all fields not present in the original config file are populated with their default values. When merged via merge_values, these default values will overwrite settings from previous config files in the config_files list. For example, if the first file disables diagnostics and the second file doesn't mention diagnostics, the second file's default (enabled) will overwrite the first file's setting. Consider merging the raw flattened JSON values and only resolving the paths within those values, or implementing a merge strategy that ignores default values.
| for partial_emmyrc in partial_emmyrcs { | ||
| config_jsons.push(partial_emmyrc); | ||
| if has_config { | ||
| merge_values(&mut merged_emmyrc_json, partial_emmyrc); | ||
| } else { | ||
| merged_emmyrc_json = partial_emmyrc; | ||
| has_config = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The partial_emmyrcs are merged without being flattened. If a partial config (e.g., from LSP client settings) uses dotted keys like "workspace.library", it will not merge correctly with the already-flattened file configurations, and the final Emmyrc will ignore these settings. This is a regression from the previous implementation which flattened the entire merged result at the end.
for partial_emmyrc in partial_emmyrcs {
let partial_emmyrc = FlattenConfigObject::parse(partial_emmyrc).to_emmyrc();
if has_config {
merge_values(&mut merged_emmyrc_json, partial_emmyrc);
} else {
merged_emmyrc_json = partial_emmyrc;
has_config = true;
}
}| ]; | ||
|
|
||
| let mut emmyrc = load_configs(vec![shared_config, workspace_config], None); | ||
| emmyrc.pre_process_emmyrc(&workspace_root); |
There was a problem hiding this comment.
This call to pre_process_emmyrc is redundant for file-backed configurations because load_configs now performs per-file preprocessing internally. While it might still be needed to resolve paths in partial_emmyrcs relative to the workspace root, in this specific test case (where paths are already absolute after the first pass), it has no effect and can be removed for clarity.
lewis6991
force-pushed
the
split/config-loading-paths
branch
from
3712627 to
c06b1f1
Compare
Preprocess each config file while its declaring directory is still known, then write back only the resulting path changes before merging. This keeps library/package/ignoreDir/resource path resolution correct without materializing defaults into unrelated fields. Keep the raw single-file config loader for edit commands and cover the sparse merge behavior with config loader regressions.
lewis6991
force-pushed
the
split/config-loading-paths
branch
from
c06b1f1 to
d21fa34
Compare
1 participant
Resolve relative workspace paths before merged runtime config loses the
declaring file context. Keep the raw single-file config loader for edit
commands, and cover parent library/package paths plus entry-local
ignoreDir preprocessing.