refactor: better file organization for CLI crates#1497
refactor: better file organization for CLI crates#1497HerringtonDarkholme merged 11 commits intomainfrom
Conversation
WalkthroughThe changes involve a significant refactoring of import statements across multiple files in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Utils
participant Worker
User->>CLI: Input Command
CLI->>Utils: Process Input
Utils->>Worker: Handle Multi-threading
Worker-->>Utils: Return Results
Utils-->>CLI: Display Output
CLI-->>User: Show Results
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (7)
crates/cli/src/lang/lang_globs.rs (2)
Line range hint
12-15: Consider reducing the use of unsafe code.The use of
unsafeblocks and mutable static variables (LANG_GLOBS) introduces potential thread-safety issues and increases the risk of undefined behavior. Consider refactoring this part of the code to use safe Rust patterns, such as:
- Using a
lazy_staticoronce_cellfor initialization ofLANG_GLOBS.- Wrapping
LANG_GLOBSin a thread-safe container likeMutexorRwLock.- Using interior mutability patterns with
RefCellif single-threaded access is guaranteed.These changes would improve the safety and maintainability of the code without sacrificing functionality.
Also applies to: 18-22
Line range hint
29-30: Address TODO comment.There's a TODO comment indicating the need for adding a test. It's important to ensure that all functionality is properly tested, especially when dealing with language-specific features.
Would you like me to help draft a test case or create a GitHub issue to track this task?
crates/cli/src/utils/worker.rs (1)
71-71: Unclear or incorrect comment.The comment
// use write to avoid send/sync trait boundis unclear or may be incorrect, as there is no usage ofwritein this context. Consider clarifying the comment or removing it if it's not relevant.Apply this diff to remove the unclear comment:
-// use write to avoid send/sync trait boundcrates/cli/src/utils/mod.rs (3)
68-81: Handle unexpected input more gracefully inpromptfunctionCurrently, if the user inputs an unrecognized command, the function loops indefinitely after displaying "Unrecognized command, try again?". Consider adding a condition to allow users to exit the prompt gracefully, such as recognizing a special exit character (e.g., 'q' for quit) or implementing a maximum retry limit to prevent potential infinite loops.
90-91: Consider logging when files are skipped due to size constraintsWhen a file is skipped because it is too large or empty, logging this event can help users understand why certain files are not being processed. This is especially useful in large batches where missing files might otherwise go unnoticed.
31-41: Handle non-key events inread_charfunctionThe
read_charfunction only handlesEvent::Keyevents and ignores other types of events. While this may be acceptable, consider handling or explicitly ignoring other event types (likeEvent::MouseorEvent::Resize) to make the code more robust and maintainable.crates/cli/src/utils/args.rs (1)
132-137: Add documentation comments for public methodsFor better maintainability and readability, consider adding documentation comments to the public methods in
OutputArgsandInputArgs. This helps other developers understand the purpose and usage of these methods.For example:
/// Determines if an interactive session is required based on the provided flags. pub fn needs_interactive(&self) -> bool { self.interactive || self.update_all }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- crates/cli/src/completions.rs (1 hunks)
- crates/cli/src/config.rs (1 hunks)
- crates/cli/src/lang/injection.rs (1 hunks)
- crates/cli/src/lang/lang_globs.rs (1 hunks)
- crates/cli/src/lib.rs (1 hunks)
- crates/cli/src/lsp.rs (1 hunks)
- crates/cli/src/new.rs (1 hunks)
- crates/cli/src/print/interactive_print.rs (1 hunks)
- crates/cli/src/run.rs (1 hunks)
- crates/cli/src/scan.rs (1 hunks)
- crates/cli/src/utils.rs (0 hunks)
- crates/cli/src/utils/args.rs (1 hunks)
- crates/cli/src/utils/mod.rs (1 hunks)
- crates/cli/src/utils/worker.rs (1 hunks)
- crates/cli/src/verify.rs (1 hunks)
- crates/cli/src/verify/find_file.rs (1 hunks)
💤 Files with no reviewable changes (1)
- crates/cli/src/utils.rs
🔇 Additional comments (12)
crates/cli/src/completions.rs (1)
29-29: LGTM: Import statement updated correctly.The import statement has been updated to reflect the new location of
ErrorContextin theutilsmodule. This change appears to be part of a larger refactoring effort to improve code organization.To ensure consistency across the codebase, please run the following script to verify that all other files using
ErrorContexthave been updated:If the first command returns any results, those files may need to be updated. The second command should show all the correct imports, including this file.
✅ Verification successful
Verification Successful: All
ErrorContextimports updated to theutilsmodule.All instances of
ErrorContexthave been correctly imported from theutilsmodule. No issues found with the import changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all files using ErrorContext import it from the utils module # Test: Search for any remaining imports from the error module rg --type rust 'use crate::error::ErrorContext' # Test: Confirm that all ErrorContext imports now come from the utils module rg --type rust 'use crate::utils::ErrorContext'Length of output: 891
crates/cli/src/lang/lang_globs.rs (1)
8-8: LGTM: Import statement updated correctly.The import statement has been updated to reflect the new location of
ErrorContextin theutilsmodule, which is consistent with the refactoring objectives.To ensure this change has been consistently applied across the codebase, please run the following script:
✅ Verification successful
✅ Verified: All import statements for
ErrorContexthave been correctly updated to theutilsmodule across the codebase.No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all imports of ErrorContext now use the utils module # Test 1: Check for any remaining imports from the error module echo "Checking for any remaining imports from the error module:" rg --type rust 'use crate::error::ErrorContext' # Test 2: Verify that all imports now use the utils module echo "Verifying that all imports now use the utils module:" rg --type rust 'use crate::utils::ErrorContext' # Note: If Test 1 returns results or Test 2 doesn't find any matches, manual review may be needed.Length of output: 1131
crates/cli/src/verify/find_file.rs (1)
3-3: LGTM! Verify consistent updates across the codebase.The import change from
crate::error::ErrorContexttocrate::utils::ErrorContextaligns with the PR objective of better file organization. This change suggests that theErrorContexttype has been moved from theerrormodule to theutilsmodule.To ensure consistency across the codebase, please run the following script to verify that all imports of
ErrorContexthave been updated:This will help identify any files that may have been missed during the refactoring process.
✅ Verification successful
Verified! The
ErrorContextimports have been successfully updated across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all imports of ErrorContext have been updated # Test: Search for any remaining imports from the old location rg --type rust 'use crate::error::ErrorContext' # Test: Confirm new imports are used consistently rg --type rust 'use crate::utils::ErrorContext'Length of output: 891
crates/cli/src/config.rs (1)
2-2: LGTM: Import statement updated correctly.The import of
ErrorContexthas been updated to use theutilsmodule instead of theerrormodule. This change is consistent with the refactoring mentioned in the PR objectives.Let's verify that this change is consistent across the codebase:
✅ Verification successful
To ensure that
ErrorContextis correctly defined and re-exported within theutilsmodule, please run the following scripts:
Verified: Import statement update is consistent across the codebase.
The
ErrorContextis correctly defined inutils/error_context.rsand re-exported inutils/mod.rs. All imports and usages have been updated to usecrate::utils::ErrorContext. No remaining references tocrate::error::ErrorContextwere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports of ErrorContext from the error module # Test 1: Search for imports of ErrorContext from the error module echo "Checking for imports of ErrorContext from the error module:" rg --type rust 'use crate::error::ErrorContext' # Test 2: Verify that ErrorContext is defined in the utils module echo "Verifying ErrorContext definition in the utils module:" rg --type rust 'pub (struct|enum|type) ErrorContext' crates/cli/src/utils.rs # Test 3: Check for other uses of ErrorContext to ensure they're updated echo "Checking for other uses of ErrorContext:" rg --type rust 'ErrorContext'Length of output: 2579
Script:
#!/bin/bash # Description: Verify the definition and re-export of ErrorContext in the utils module # Test 4: Verify that ErrorContext is defined in utils/error_context.rs echo "Verifying ErrorContext definition in utils/error_context.rs:" rg --type rust 'pub (struct|enum|type) ErrorContext' crates/cli/src/utils/error_context.rs # Test 5: Check if ErrorContext is re-exported in utils/mod.rs echo "Checking if ErrorContext is re-exported in utils/mod.rs:" rg --type rust 'pub use .*ErrorContext' crates/cli/src/utils/mod.rsLength of output: 496
crates/cli/src/print/interactive_print.rs (1)
4-4: LGTM! Verify impact on other files.The change in the import statement is correct and aligns with the PR objectives of refactoring for better file organization. The use of the alias
ECremains consistent, which should prevent any breaking changes in this file.To ensure this change doesn't introduce any issues in other parts of the codebase, please run the following script:
✅ Verification successful
Verified! The import of
ErrorContextfromcrate::utilsis correct and aligns with the refactoring.ErrorContextis defined as an enum incrates/cli/src/utils/error_context.rs, and all imports have been updated accordingly, ensuring no remaining references tocrate::error::ErrorContext.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of moving ErrorContext from error to utils module # Test 1: Check for any remaining imports of ErrorContext from the error module echo "Checking for remaining imports of ErrorContext from error module:" rg --type rust 'use crate::error::ErrorContext' # Test 2: Verify that all imports of ErrorContext now come from the utils module echo "Verifying imports of ErrorContext from utils module:" rg --type rust 'use crate::utils::ErrorContext' # Test 3: Check for any potential naming conflicts in the utils module echo "Checking for potential naming conflicts in utils module:" rg --type rust '^(pub )?struct ErrorContext' crates/cli/src/utils.rs # Test 4: Verify that the ErrorContext struct is no longer present in the error module echo "Verifying ErrorContext is no longer in error module:" rg --type rust '^(pub )?struct ErrorContext' crates/cli/src/error.rsLength of output: 1655
Script:
#!/bin/bash # Description: Locate the definition of ErrorContext in the codebase echo "Searching for the definition of ErrorContext struct:" rg --type rust 'struct ErrorContext' --glob '!(node_modules|dist|build)/**'Length of output: 192
Script:
#!/bin/bash # Description: Locate all definitions of ErrorContext in the codebase echo "Searching for the definition of ErrorContext as a struct:" rg --type rust 'struct ErrorContext' --glob '!(node_modules|dist|build)/**' echo "Searching for the definition of ErrorContext as an enum:" rg --type rust 'enum ErrorContext' --glob '!(node_modules|dist|build)/**' echo "Searching for type aliases of ErrorContext:" rg --type rust 'type ErrorContext' --glob '!(node_modules|dist|build)/**' echo "Searching for any implementation blocks of ErrorContext:" rg --type rust 'impl ErrorContext' --glob '!(node_modules|dist|build)/**' echo "Searching for the definition of ErrorContext using different patterns:" rg --type rust 'ErrorContext\s*::' --glob '!(node_modules|dist|build)/**' | rg -B 5 'ErrorContext'Length of output: 1833
crates/cli/src/verify.rs (1)
9-9: LGTM: Import change aligns with refactoring objectives.The change from
use crate::error::ErrorContexttouse crate::utils::ErrorContextaligns with the PR objective of better file organization. This suggests thatErrorContextis now part of a more general utility module, which could improve code reusability and organization.To ensure consistency across the codebase, please run the following script:
This script will help identify any inconsistencies in
ErrorContextimports across the codebase, ensuring that all files have been updated correctly.✅ Verification successful
All imports of
ErrorContexthave been successfully migrated to theutilsmodule. No remaining imports from theerrormodule were found, and all usages within the codebase are consistent with the refactoring objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all imports of ErrorContext are from the utils module # Test 1: Check for any remaining imports from the error module echo "Checking for imports from error module:" rg --type rust 'use crate::error::ErrorContext' # Test 2: Verify new imports from the utils module echo "Verifying imports from utils module:" rg --type rust 'use crate::utils::ErrorContext' # Test 3: Check for any uses of ErrorContext without an import echo "Checking for uses of ErrorContext without import:" rg --type rust 'ErrorContext' -g '!verify.rs' | rg -v 'use crate::(utils|error)::ErrorContext'Length of output: 2418
crates/cli/src/scan.rs (1)
18-18: LGTM! Verify other imports ofErrorContext.The change to import
ErrorContextfromcrate::utilsinstead ofcrate::errorlooks good and aligns with the refactoring mentioned in the PR objectives. This change improves the organization of the codebase.To ensure consistency across the codebase, please run the following script to verify that all other files importing
ErrorContexthave been updated:crates/cli/src/run.rs (2)
14-14: Verify completeness of ErrorContext in new locationWhile the import change looks good, it's important to ensure that all the error contexts used in this file (
ParsePattern,LanguageNotSpecified,PatternHasError) are still defined in the new location ofErrorContext.Please run the following script to verify the completeness of the
ErrorContextenum:#!/bin/bash # Description: Verify that all used error contexts are defined in the new ErrorContext location # Test: Check if all used error contexts are defined in utils module echo "Checking if all used error contexts are defined in utils module:" rg --type rust -e 'ParsePattern' -e 'LanguageNotSpecified' -e 'PatternHasError' crates/cli/src/utils.rsThis script will help ensure that all the error contexts used in this file are properly defined in the new location of
ErrorContextin theutilsmodule.
14-14: LGTM! Verify consistency across the codebase.The import statement for
ErrorContexthas been updated to use theutilsmodule instead of theerrormodule. This change looks good, as it's consistent with the refactoring mentioned in the PR objectives.To ensure this change is consistent across the codebase, please run the following script:
This script will help ensure that:
- There are no remaining imports of
ErrorContextfrom theerrormodule.- All imports of
ErrorContextare now from theutilsmodule.- The
ErrorContexttype is actually defined in theutilsmodule.✅ Verification successful
Verification Required: Refining the Import Path for
ErrorContextThe initial verification encountered issues:
- The
crates/cli/src/utils.rsfile does not exist.- There was a syntax error in the shell script.
To ensure that
ErrorContextis correctly imported from its new location, please run the following updated script:This script will:
- Ensure there are no lingering imports of
ErrorContextfrom theerrormodule.- Confirm that all imports of
ErrorContextare now from theutilsmodule.- Locate the definition of
ErrorContextwithin the codebase to verify it exists and is correctly placed.Please execute the above script and provide the results for further verification.
Verified:
ErrorContextImport Consistency ConfirmedAll import statements for
ErrorContexthave been successfully updated to use theutilsmodule, and theErrorContexttype is correctly defined incrates/cli/src/utils/error_context.rs. No remaining imports from theerrormodule were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ErrorContext is now imported from utils module consistently # Test 1: Check for any remaining imports from error module echo "Checking for any remaining imports from error module:" rg --type rust 'use crate::error::ErrorContext' # Test 2: Verify new imports from utils module echo "Verifying new imports from utils module:" rg --type rust 'use crate::utils::ErrorContext' # Test 3: Check if ErrorContext is defined in utils module echo "Checking if ErrorContext is defined in utils module:" rg --type rust 'pub (struct|enum|type) ErrorContext' crates/cli/src/utils.rsLength of output: 397
Script:
#!/bin/bash # Description: Corrected scripts to verify the import and definition of ErrorContext. # Test 1: Check for any remaining imports from the error module echo "Checking for any remaining imports from the error module:" rg --type rust 'use crate::error::ErrorContext' # Test 2: Verify new imports from the utils module echo "Verifying new imports from the utils module:" rg --type rust 'use crate::utils::ErrorContext' # Test 3: Search for the definition of ErrorContext across the entire codebase echo "Searching for the definition of ErrorContext in the codebase:" rg --type rust 'pub (struct|enum|type) ErrorContext'Length of output: 1365
crates/cli/src/new.rs (2)
Line range hint
1-465: Summary: Minor import change with no functional impact.The only change in this file is the update to the import statement for
ErrorContext. This change is part of the larger refactoring effort to improve file organization for CLI crates, as mentioned in the PR objectives. The update doesn't affect the functionality of the code, and all usages ofErrorContext(aliased asEC) remain valid throughout the file.No further changes are required in this file as a result of this update.
6-6: LGTM: Import statement updated correctly.The import of
ErrorContexthas been correctly updated to use theutilsmodule instead of theerrormodule. This change aligns with the refactoring mentioned in the PR objectives.To ensure consistency across the codebase, let's verify that there are no remaining imports of
ErrorContextfrom theerrormodule:crates/cli/src/utils/mod.rs (1)
195-216: Great job including unit tests for language embeddingThe tests
test_html_embeddingandtest_html_embedding_lang_not_foundeffectively verify the functionality of HTML script language embedding and the behavior when the language is not recognized. These tests enhance code reliability and maintainability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1497 +/- ##
==========================================
- Coverage 72.44% 72.43% -0.02%
==========================================
Files 84 86 +2
Lines 5887 5888 +1
==========================================
Hits 4265 4265
- Misses 1622 1623 +1 ☔ View full report in Codecov by Sentry. |
94bd341 to
b33911e
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Herrington Darkholme <2883231+HerringtonDarkholme@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
crates/cli/src/utils/worker.rs (1)
69-77: Clarify comment about send/sync trait boundsThe
Items::oncemethod is well-implemented for creating a single-item channel. However, the comment on line 71 needs clarification.Consider updating the comment to explain why using
sendwould require send/sync trait bounds and howwriteavoids this. This will help future maintainers understand the reasoning behind this implementation choice. For example:- // use write to avoid send/sync trait bound + // Use `send` instead of `write` to avoid requiring `T: Send + Sync`. + // `send` moves the value into the channel, while `write` copies it, + // allowing us to handle types that aren't `Send + Sync`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- crates/cli/src/utils/args.rs (1 hunks)
- crates/cli/src/utils/mod.rs (1 hunks)
- crates/cli/src/utils/worker.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/cli/src/utils/args.rs
- crates/cli/src/utils/mod.rs
🧰 Additional context used
📓 Learnings (1)
crates/cli/src/utils/worker.rs (1)
Learnt from: HerringtonDarkholme PR: ast-grep/ast-grep#1497 File: crates/cli/src/utils/worker.rs:99-126 Timestamp: 2024-09-29T21:27:54.038Z Learning: In the `run_worker` function, the `worker` run will only consume one additional thread, and most thread creation is managed by `ignore::WalkParallel`.
🔇 Additional comments (4)
crates/cli/src/utils/worker.rs (4)
15-22: LGTM: Well-defined Worker traitThe
Workertrait is well-designed and documented. It clearly defines the interface for workers in the multi-producer-single-consumer pattern, with an associatedItemtype and aconsume_itemsmethod.
30-42: LGTM: Well-structured PathWorker traitThe
PathWorkertrait extendsWorkerwith file system-specific operations. It's well-documented and provides a default implementation forrun_path, promoting code reuse.
99-127: LGTM: Efficient worker implementationThe
run_workerfunction effectively implements the multi-producer-single-consumer pattern using channels and thread spawning. The use ofArcfor sharing the worker across threads is appropriate.As noted in the learnings, the worker run will only consume one additional thread, with most thread creation managed by
ignore::WalkParallel. This addresses previous concerns about potential excessive thread usage.
1-127: LGTM: Well-structured and organized worker implementationThe overall structure and organization of this file are excellent. It introduces multiple traits and structs related to worker functionality in a clear and logical order. Each component is well-documented, and the implementation follows Rust best practices and idioms.
The file effectively encapsulates the worker-related functionality, providing a solid foundation for multi-threaded processing in the CLI application.
Now the root folder for cli contains only cli command.
Change:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
ErrorContextfrom theutilsmodule instead of theerrormodule across multiple files.Chores