Refactor search command #912
Conversation
WalkthroughThe changes in this pull request involve updates to several files, primarily focusing on the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/commands/search.rs (3)
70-79: Simplify redundant checks inmatch_binariesmethod.The
filter_mapin thematch_binariesmethod redundantly checkspattern.is_match(&b)after already filtering for matches. This can be simplified to improve readability and efficiency.Apply this diff to simplify the method:
fn match_binaries(&mut self, pattern: &Regex, binaries: Vec<String>) -> &mut Self { let binary_matches = binaries .into_iter() - .filter(|binary| pattern.is_match(binary)) - .filter_map(|b| { - if pattern.is_match(&b) { - Some(b.clone()) - } else { - None - } - }); + .filter(|binary| pattern.is_match(binary)) + .collect(); self.bins.extend(binary_matches); self }
129-129: Usepattern.as_str()instead ofpattern.to_string()for exact match comparison.Comparing
manifest.name()topattern.to_string()may not yield the expected result becausepattern.to_string()includes the regex syntax, which may not match the manifest name as intended. Usepattern.as_str()to compare the raw pattern string.Apply this diff:
let exact_match = unsafe { manifest.name() } == pattern.to_string(); +let exact_match = unsafe { manifest.name() } == pattern.as_str();
251-256: Avoid variable shadowing for improved clarity.The variable
bucketis shadowed within theif let Some(bucket) = self.bucketblock, which can cause confusion and potential errors. Consider renaming the inner variable to avoid shadowing the outerbucket.Apply this diff to rename the inner variable:
if let Some(bucket_flag) = self.bucket { warning!("Using bucket flag instead of bucket/package syntax"); bucket_flag } else { bucket.to_string() }CHANGELOG.md (1)
22-22: Specify the noun after "each" for clarity.The sentence "Download hash checks now report to a progress bar rather than a print message for each" is missing the noun after "each". Consider adding the noun to clarify the message.
Apply this diff:
- Download hash checks now report to a progress bar rather than a print message for each + Download hash checks now report to a progress bar rather than a print message for each file🧰 Tools
🪛 LanguageTool
[grammar] ~22-~22: Use a singular noun after the quantifier ‘each’, or change it to “all”.
Context: ...ess bar rather than a print message for each - Logs will now go into<PWD>/logsif runnin...(EACH_EVERY_NNS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)Cargo.toml(1 hunks)src/commands/app/download.rs(1 hunks)src/commands/search.rs(6 hunks)src/commands/virustotal.rs(1 hunks)src/main.rs(2 hunks)src/output.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~22-~22: Use a singular noun after the quantifier ‘each’, or change it to “all”.
Context: ...ess bar rather than a print message for each - Logs will now go into <PWD>/logs if runnin...
(EACH_EVERY_NNS)
🔇 Additional comments (7)
src/commands/search.rs (1)
45-49:
Missing pattern parameter in MatchCriteria::matches function.
The pattern variable is used within the matches method but is not included in the function signature. This will result in a compilation error due to the undefined pattern.
Apply this diff to add the missing pattern parameter:
pub fn matches(
file_name: &str,
+ pattern: &Regex,
list_binaries: impl FnOnce() -> Vec<String>,
mode: SearchMode,
) -> Self {Likely invalid or redundant comment.
src/output.rs (1)
16-24: LGTM!
The warning! macro is correctly defined, enabling consistent and formatted warning messages throughout the codebase.
src/commands/app/download.rs (1)
42-43: LGTM!
Using #[clap(from_global)] for the arch field allows for consistent architecture settings across commands and enhances configurability.
src/main.rs (2)
30-33: LGTM: Clean import organization
The imports are well-organized and the new Architecture import is properly placed within the sprinkles module group.
124-131: LGTM: Well-structured architecture configuration
The new arch field is:
- Properly marked as global for consistent usage across commands
- Has clear documentation explaining its purpose
- Uses a sensible default value
src/commands/virustotal.rs (2)
115-115: LGTM: Proper global architecture configuration
The arch field correctly uses from_global to inherit the architecture setting from the main Args struct, maintaining consistency across commands.
Line range hint 177-184: Verify architecture handling in install_config
The architecture is used consistently in install_config calls. However, let's verify the error handling for unsupported architectures.
✅ Verification successful
Let me gather more information about the architecture handling by checking the implementation of install_config and related error handling.
Let me try a different approach to find architecture-related code and error handling.
Let me check one more aspect to understand how architecture validation is handled in the manifest and configuration code.
Based on the code analysis, I can now generate the final response:
Architecture handling in install_config appears to be properly implemented
The code shows that architecture handling in install_config is implemented through a consistent pattern across multiple commands (search, virustotal, info). The architecture parameter is properly passed through to configuration merging (merge_default) and there's no evidence of unhandled architecture cases. The code gracefully handles both hash and URL configurations for different architectures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if install_config handles unsupported architectures gracefully
# Search for error handling around install_config calls
rg -A 5 "install_config.*arch"
# Look for architecture validation or error handling patterns
ast-grep --pattern 'install_config($_) {
$$$
if $_ {
$$$
}
$$$
}'
Length of output: 1677
Script:
#!/bin/bash
# Search for install_config implementation and architecture-related error handling
ast-grep --pattern 'fn install_config($_) {
$$$
}'
# Look for architecture-related structs and implementations
rg -A 5 "pub struct.*arch|impl.*arch"
# Search for architecture validation or error patterns
rg -A 3 "unsupported.*arch|invalid.*arch|unknown.*arch"
Length of output: 152
Script:
#!/bin/bash
# Search for architecture-related code in manifest handling
rg -A 5 "manifest.*arch|Arch|Architecture"
# Look for error handling in manifest-related code
ast-grep --pattern 'match $_ {
$$$
}'
# Find where architecture is used in configuration
rg -A 3 "config.*arch|arch.*config"
Length of output: 40676
Script:
#!/bin/bash
# Look for architecture-related code in manifest and configuration
ast-grep --pattern 'struct $_Config {
$$$
}'
# Search for architecture validation or error handling in manifest code
rg -A 5 "fn.*manifest.*arch|manifest.*arch.*fn"
# Look for any architecture-related error types or handling
rg -A 3 "Error.*arch|arch.*error"
Length of output: 533
Summary by CodeRabbit
New Features
app download --outdatedfor downloading outdated applications.jsonflag in the search command.Bug Fixes
[installed]label correctly.Documentation
CHANGELOG.mdto reflect notable changes in the project.Chores
1.15.0to1.16.0.sfsu_macroscrate from the project.warning!macro for colored warning messages.