feat: unify configuration reading#1560
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the handling of project configurations within the command-line interface of the application. The Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1560 +/- ##
==========================================
+ Coverage 72.62% 72.63% +0.01%
==========================================
Files 88 88
Lines 6035 6034 -1
==========================================
Hits 4383 4383
+ Misses 1652 1651 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
crates/cli/src/lsp.rs (2)
12-12: Address TODO comment regarding error handlingThe comment "TODO: move this error to client" suggests there's pending work on error handling strategy.
Would you like me to help implement proper error propagation to the LSP client or create a GitHub issue to track this task?
Line range hint
13-31: Consider enhancing error messages for better debuggingWhile the error handling is functional, consider adding more context to error messages to help users troubleshoot configuration issues more effectively.
let config_result_std: std::result::Result<_, String> = config_result .map_err(|e| { // convert anyhow::Error to String with chain of causes - e.chain() - .map(|e| e.to_string()) - .collect::<Vec<_>>() - .join(". ") + format!("Failed to load LSP configuration: {}", + e.chain() + .map(|e| e.to_string()) + .collect::<Vec<_>>() + .join(". ")) })crates/cli/src/config.rs (1)
Line range hint
93-105: Consider simplifying the nested Result return type.The change from
Result<Option<Self>>toResult<Result<Self>>introduces nested error handling which could make the API more complex to use. While this change aligns with the PR's goal of unifying configuration handling, consider these alternatives:
- Use a custom error type that distinguishes between IO errors and configuration errors
- Flatten the results using
Result<Self>with a custom error enumExample implementation using a custom error type:
#[derive(Debug)] pub enum ConfigError { IoError(std::io::Error), ProjectNotExist, // ... other error variants } impl From<std::io::Error> for ConfigError { fn from(err: std::io::Error) -> Self { ConfigError::IoError(err) } } pub fn setup(config_path: Option<PathBuf>) -> Result<Self, ConfigError> { let Some((project_dir, mut sg_config)) = Self::discover_project(config_path)? else { return Err(ConfigError::ProjectNotExist); }; // ... rest of the implementation }This approach would:
- Simplify error handling for callers
- Maintain clear distinction between error types
- Improve API ergonomics
crates/cli/src/lib.rs (2)
44-46: LGTM! Consider enhancing the documentation.The configuration field implementation looks good. The type choice and CLI argument setup are appropriate.
Consider expanding the documentation to include:
- The expected format of the config file (YAML)
- The default config path behavior
- /// Path to ast-grep root config, default is sgconfig.yml. + /// Path to ast-grep root config file in YAML format. + /// If not specified, defaults to 'sgconfig.yml' in the current directory.
205-205: Enhance test coverage for configuration handling.While the basic config flag test is good, consider adding more test cases to ensure robust configuration handling:
Add test cases for:
- Config flag with other commands (scan, test, new, lsp)
- Default config path behavior
- Invalid config paths
- Config file format validation
Example additions:
#[test] fn test_config_handling() { ok("scan -c custom_config.yml"); ok("test -c custom_config.yml"); error("scan -c"); // missing config path error("scan -c invalid/path/config.yml"); // non-existent config }crates/cli/src/new.rs (1)
374-394: Consider adding negative test casesWhile the current tests verify successful creation scenarios, consider adding test cases for:
- Invalid configuration paths
- Permission denied scenarios
- Duplicate entity creation attempts
Example test case:
#[test] fn test_create_rule_invalid_config() -> Result<()> { let dir = TempDir::new()?; let invalid_config = dir.path().join("nonexistent.yml"); let project = ProjectConfig::setup(Some(invalid_config)); assert!(project.is_err()); Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- crates/cli/src/config.rs (2 hunks)
- crates/cli/src/lib.rs (5 hunks)
- crates/cli/src/lsp.rs (3 hunks)
- crates/cli/src/new.rs (3 hunks)
- crates/cli/src/run.rs (0 hunks)
- crates/cli/src/scan.rs (8 hunks)
- crates/cli/src/verify.rs (2 hunks)
💤 Files with no reviewable changes (1)
- crates/cli/src/run.rs
🔇 Additional comments (14)
crates/cli/src/lsp.rs (3)
8-8: LGTM: Simplified argument structure aligns with unified configuration approachThe removal of the config field from LspArg supports the PR's objective of unifying configuration discovery across subcommands.
33-38: LGTM: Proper async runtime handling with error contextThe function correctly sets up the tokio runtime and provides good error context for runtime creation failures.
47-50: Enhance test coverage for LSP functionalityWhile the error case is tested, consider adding the following test cases:
- Successful configuration loading
- Various configuration error scenarios
- LSP server initialization with valid config
Also, the test is currently ignored. Consider implementing the pending LSP tests or document why they're blocked.
crates/cli/src/config.rs (1)
93-95: Verify error handling in all callers.The change in error handling strategy (from
OptiontoResult) requires careful verification of all calling code to ensure proper error handling.Let's verify the impact:
✅ Verification successful
Let me gather more context about the error handling in these files to make a proper assessment.
Let me check one more thing to verify if these are test functions since the last command didn't show the test modules clearly.
Based on the gathered information, I can now provide the final response:
Error handling is properly implemented across all callers
The error handling changes in
ProjectConfig::setupare handled appropriately:
- In
lib.rs: Uses proper error propagation with?operator- In
scan.rs: The.unwrap()usage is within a test functiontest_run_with_config- In
new.rs: Both calls use proper error propagation with?operator and are within test functionsThe only instance of
.unwrap()is in test code, while production code properly propagates errors, making this change safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all callers of ProjectConfig::setup to verify error handling # Search for setup method calls echo "Searching for ProjectConfig::setup callers:" rg -p "ProjectConfig::setup" --type rust # Search for potential error handling patterns echo -e "\nVerifying error handling patterns:" rg -p "setup\(.*\)\.(?:and_then|map_err|unwrap|expect)" --type rustLength of output: 789
Script:
#!/bin/bash # Get context around the setup calls to understand error handling echo "Context from lib.rs:" rg -p "ProjectConfig::setup" -B 5 -A 5 --type rust crates/cli/src/lib.rs echo -e "\nContext from scan.rs:" rg -p "ProjectConfig::setup" -B 5 -A 5 --type rust crates/cli/src/scan.rs echo -e "\nContext from new.rs:" rg -p "ProjectConfig::setup" -B 5 -A 5 --type rust crates/cli/src/new.rs # Check if these are test functions echo -e "\nChecking for test modules:" rg -p "mod tests" --type rust crates/cli/src/scan.rs crates/cli/src/new.rsLength of output: 2120
Script:
#!/bin/bash # Check for test attributes and modules echo "Checking for test attributes in scan.rs:" rg -p "#\[test\]" -B 10 -A 2 crates/cli/src/scan.rs echo -e "\nChecking for test attributes in new.rs:" rg -p "#\[test\]" -B 10 -A 2 crates/cli/src/new.rs # Also check the actual error handling in lib.rs echo -e "\nChecking error handling in lib.rs:" rg -p "fn run" -B 2 -A 10 crates/cli/src/lib.rsLength of output: 2695
crates/cli/src/lib.rs (1)
110-113: LGTM! Clean implementation of unified configuration handling.The changes consistently pass the project configuration to command handlers, aligning well with the PR's objective of unifying configuration discovery across subcommands.
crates/cli/src/verify.rs (2)
186-187: LGTM! Function signature change aligns with configuration unification.The updated signature properly handles project configuration through
Result<ProjectConfig>, aligning with the PR's objective to unify configuration reading across subcommands. The error handling with the?operator follows Rust best practices.
314-314: LGTM! Test case properly verifies error handling.The test case correctly verifies the error path for the new configuration handling.
crates/cli/src/new.rs (3)
147-151: LGTM: Improved error handling with Result typeThe change to accept
Result<ProjectConfig>enhances error handling and aligns with the PR's objective to unify configuration handling across subcommands.
155-157: LGTM: Clean error handling implementationThe error handling implementation follows Rust idioms and properly differentiates between project creation and other entity types.
Line range hint
1-420: Verify removal of direct config path referencesLet's ensure all direct config path references have been properly migrated to use the new unified configuration approach.
✅ Verification successful
Based on the search results, I can now generate the final response:
All config path references are properly handled
The search results confirm that all config path references in the codebase are properly handled through the unified configuration approach. The occurrences found are either:
- Documentation strings explaining the config file usage
- Test fixtures setting up test environments
- Proper usage of the new configuration system through
ProjectConfig::setup- Error messages guiding users about configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct references to config files or paths # that might need to be updated to use the new unified approach # Look for potential config-related string literals rg -g '*.rs' -i 'config(\.yml|\.yaml|_path|file)' # Look for potential direct path construction for config files rg -g '*.rs' 'Path.*config'Length of output: 4673
crates/cli/src/scan.rs (4)
Line range hint
66-85: Refactoredrun_with_configfunction correctly handlesprojectThe updates to
run_with_configto acceptproject: Result<ProjectConfig>ensure that configuration errors are properly propagated. The changes are consistent and enhance error handling across the function.
89-101:run_scanfunction's signature update is appropriateThe
run_scanfunction now acceptsproject: Result<ProjectConfig>, aligning it with the changes inrun_with_configand ensuring consistent error handling throughout the scanning process.
Line range hint
111-122: Proper error propagation inScanWithConfig::try_newThe use of the
?operator onprojectinScanWithConfig::try_newcorrectly propagates any errors in the project configuration, ensuring that issues are surfaced early in the initialization process.
Line range hint
417-428: Ensure consistent error handling in test functionsIn the test functions
test_scan_with_inline_rulesandtest_scan_with_inline_rules_diff, you're passingErr(anyhow::anyhow!("not found"))as theprojectparameter. Verify that this aligns with the intended test scenarios and that error cases are appropriately handled withinrun_with_config.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/cli/src/lib.rs (1)
106-106: Consider improving error handling for project setup.The project setup error is silently passed through. Consider handling configuration errors explicitly to provide better user feedback when the config file is invalid or missing.
- let project = setup_project_is_possible(&args); + let project = setup_project_is_possible(&args).map_err(|e| { + anyhow::anyhow!("Failed to load configuration: {}", e) + })?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- crates/cli/src/lib.rs (5 hunks)
🔇 Additional comments (4)
crates/cli/src/lib.rs (4)
14-14: LGTM: Well-structured configuration field addition.The global configuration option is properly implemented with:
- Clear documentation
- Appropriate clap attributes for global usage
- Optional type for backward compatibility
Also applies to: 17-17, 44-46
87-101: Refactor: Use clap's parsed configuration value.The manual parsing of the config flag duplicates functionality already handled by clap through the
Appstruct. This implementation:
- Adds unnecessary complexity
- Could miss edge cases that clap handles
- Is harder to maintain
174-174: LGTM: Test coverage properly updated.The test suite has been appropriately updated to:
- Verify the new error message format
- Test the global config argument functionality
Also applies to: 210-210
115-118: Verify: Should Run command use project configuration?While other commands now use project configuration, the Run command doesn't. This seems inconsistent with the PR's goal of unifying configuration handling.
fix #1557
Summary by CodeRabbit
Release Notes
New Features
-cor--configflags.Bug Fixes
Refactor
Resulttypes, enhancing error management.Tests