feat(config): load imports from global and explicit config paths#440
feat(config): load imports from global and explicit config paths#440rpendleton wants to merge 2 commits into
Conversation
Greptile SummaryThis PR extends the Confidence Score: 5/5Safe to merge; no correctness bugs found in the changed paths. All P0/P1 findings from prior review rounds (non-recursive nested imports) have been acknowledged. The new code is logically correct for the advertised single-level import behaviour, the deduplication in No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[load_smart] -->|explicit path| B[load_with_imports]
A -->|default filename| C[load_with_recursion]
C --> D[load_recursive]
D -->|per file in dir| B
D -->|root = true| E[load_global]
D -->|at FS root| E
E --> B
B --> F[load file]
F --> G{import list?}
G -->|yes| H[load_import for each]
H --> I[merge: import_config < file_config]
G -->|no| J[return config]
I --> J
Reviews (3): Last reviewed commit: "fix(config): collapse /.. to / in normal..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request improves configuration loading by ensuring imports are processed for global and explicit config files. It refactors import printing in the config-files command and adds a normalize_path utility for path deduplication, supported by new integration tests. Feedback suggests making import loading recursive to handle nested imports and improving path normalization for root directory edge cases.
| fn load_with_imports(path: &Path) -> Result<Self> { | ||
| let mut config = Self::load(path)?; | ||
| let base_dir = path.parent().unwrap_or_else(|| Path::new(".")); | ||
|
|
||
| for import_path in &config.import.clone() { | ||
| let import_config = Self::load_import(import_path, base_dir)?; | ||
| config = Self::merge_configs(import_config, config)?; | ||
| } | ||
|
|
||
| Ok(config) | ||
| } |
There was a problem hiding this comment.
The current implementation of load_with_imports only resolves one level of imports. If an imported file itself contains an import directive, those nested imports will not be loaded because load_import calls Self::load (which doesn't process imports) rather than Self::load_with_imports. While this matches the previous behavior in load_recursive, it limits the utility of imports in global and explicit config paths. Consider making this recursive, though you would need to handle potential circular imports (e.g., by passing a HashSet of visited paths).
| fn normalize_path(path: PathBuf) -> PathBuf { | ||
| let mut out = PathBuf::new(); | ||
| for component in path.components() { | ||
| match component { | ||
| Component::Prefix(_) | Component::RootDir => out.push(component), | ||
| Component::CurDir => {} | ||
| Component::ParentDir => { | ||
| let can_pop = matches!(out.components().next_back(), Some(Component::Normal(_))); | ||
| if can_pop { | ||
| out.pop(); | ||
| } else { | ||
| out.push(".."); | ||
| } | ||
| } | ||
| Component::Normal(c) => out.push(c), | ||
| } | ||
| } | ||
| if out.as_os_str().is_empty() { | ||
| PathBuf::from(".") | ||
| } else { | ||
| out | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
The normalize_path function provides lexical normalization which is useful for deduplicating paths in the HashSet. However, it doesn't handle the case where .. is applied to a root directory (e.g., /.. remains /.. instead of collapsing to /). While unlikely to affect config file discovery in practice, a more robust implementation or using a crate like path-clean (if dependencies are allowed) might be safer for general path handling.
There was a problem hiding this comment.
Fixed in 7cb803b, though I wondered if using something like path-clean would be preferable. I didn't want to add a dependency without further discussion though.
| let base_dir = path.parent().unwrap_or_else(|| Path::new(".")); | ||
|
|
||
| for import_path in &config.import.clone() { | ||
| let import_config = Self::load_import(import_path, base_dir)?; |
There was a problem hiding this comment.
One thing worth calling out on this line: since we're using load_import directly, imports aren't resolved transitively. If the imported file declares its own import, those won't be loaded. So, while this PR widens where import works (global config and explicit --config paths), it doesn't make it work everywhere.
I considered making it transitive, but that pulls in edge cases like cycle detection. If we do want to support that, a cleaner approach might be a shared config walker that does a partial parse and resolves imports recursively in one place, and then both config.rs and config_files.rs could rely on that.
For now, I figured I'd open this as a draft to discuss the direction before committing to anything.
2f9c200 to
ee15447
Compare
Previously, the import directive was only honored during recursive project config discovery. Now the global config and explicitly provided --config paths also resolve their imports, and config-files surfaces global imports too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ee15447 to
7cb803b
Compare
Previously, the import directive was only honored during recursive project config discovery. Now the global config and explicitly provided --config paths also resolve their imports, and config-files surfaces global imports too.