🧹 Refactor walk_for_completions to use SearchContext#3128
Conversation
- Fixes high complexity by replacing 5 related arguments with a `SearchContext` struct in `walk_for_completions`, `walk_always_discoverable_dirs`, and `add_local_reference_completions`. - Removes `#[allow(clippy::too_many_arguments)]` from these functions. - Simplifies method signatures and improves code maintainability. Co-authored-by: Hmbown <101357273+Hmbown@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![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 introduces a SearchContext struct to encapsulate search-related parameters, simplifying the signatures of several walk and completion functions and allowing the removal of #[allow(clippy::too_many_arguments)] attributes. The review feedback suggests further improving this design by implementing helper methods on SearchContext (specifically is_at_limit and add_match) to centralize limit checking and match categorization, which would reduce code duplication across the traversal functions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| struct SearchContext<'a> { | ||
| needle: &'a str, | ||
| limit: usize, | ||
| prefix_hits: &'a mut Vec<String>, | ||
| substring_hits: &'a mut Vec<String>, | ||
| seen: &'a mut HashSet<PathBuf>, | ||
| } |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, we can encapsulate the search matching and limit checking logic directly within SearchContext. This keeps the walk functions cleaner and more focused on traversal.
struct SearchContext<'a> {
needle: &'a str,
limit: usize,
prefix_hits: &'a mut Vec<String>,
substring_hits: &'a mut Vec<String>,
seen: &'a mut HashSet<PathBuf>,
}
impl<'a> SearchContext<'a> {
fn is_at_limit(&self) -> bool {
self.prefix_hits.len() + self.substring_hits.len() >= self.limit
}
fn add_match(&mut self, candidate: String) {
let lower = candidate.to_lowercase();
if self.needle.is_empty() || lower.starts_with(self.needle) {
self.prefix_hits.push(candidate);
} else if lower.contains(self.needle) {
self.substring_hits.push(candidate);
}
}
}| } | ||
| for entry in builder.build().flatten() { | ||
| if prefix_hits.len() + substring_hits.len() >= limit { | ||
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { |
There was a problem hiding this comment.
Use the new is_at_limit helper method on SearchContext to simplify the limit check.
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { | |
| if ctx.is_at_limit() { |
| if ctx.needle.is_empty() || lower.starts_with(ctx.needle) { | ||
| ctx.prefix_hits.push(candidate); | ||
| } else if lower.contains(ctx.needle) { | ||
| ctx.substring_hits.push(candidate); | ||
| } |
There was a problem hiding this comment.
Use the new add_match helper method on SearchContext to encapsulate the matching logic. Note that you should also remove the now-unused let lower = candidate.to_lowercase(); on the preceding line (line 437) to avoid compiler warnings.
ctx.add_match(candidate);|
|
||
| for entry in builder.build().flatten() { | ||
| if prefix_hits.len() + substring_hits.len() >= limit { | ||
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { |
There was a problem hiding this comment.
Use the new is_at_limit helper method on SearchContext to simplify the limit check.
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { | |
| if ctx.is_at_limit() { |
| if ctx.needle.is_empty() || lower.starts_with(ctx.needle) { | ||
| ctx.prefix_hits.push(candidate); | ||
| } else if lower.contains(ctx.needle) { | ||
| ctx.substring_hits.push(candidate); | ||
| } |
There was a problem hiding this comment.
Use the new add_match helper method on SearchContext to encapsulate the matching logic. Note that you should also remove the now-unused let lower = candidate.to_lowercase(); on the preceding line (line 479) to avoid compiler warnings.
ctx.add_match(candidate);|
|
||
| for path in local_reference_paths(root, LOCAL_REFERENCE_SCAN_LIMIT, max_depth) { | ||
| if prefix_hits.len() + substring_hits.len() >= limit { | ||
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { |
There was a problem hiding this comment.
Use the new is_at_limit helper method on SearchContext to simplify the limit check.
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { | |
| if ctx.is_at_limit() { |
| if ctx.needle.is_empty() || lower.starts_with(ctx.needle) { | ||
| ctx.prefix_hits.push(rel_str); | ||
| } else if lower.contains(ctx.needle) { | ||
| ctx.substring_hits.push(rel_str); | ||
| } |
There was a problem hiding this comment.
Use the new add_match helper method on SearchContext to encapsulate the matching logic. Note that you should also remove the now-unused let lower = rel_str.to_lowercase(); on the preceding line (line 515) to avoid compiler warnings.
ctx.add_match(rel_str);
Introduce a small SearchContext for workspace completion walks so the related search state moves together and the helper signatures no longer need too-many-arguments allowances. Also keeps the nearby hotbar test clippy-clean under the all-targets TUI gate. Modified harvest from PR Hmbown#3128 by @Hmbown. Co-authored-by: Hmbown <101357273+Hmbown@users.noreply.github.com>
1 participant
🎯 What: The code health issue addressed was "High Complexity: Too Many Arguments in
walk_for_completions". This was resolved by creating aSearchContextstruct that groups the 5 related search state parameters and passing it by mutable reference instead.💡 Why: How this improves maintainability: The functions
walk_for_completions,walk_always_discoverable_dirs, andadd_local_reference_completionseach took 8 arguments. Grouping 5 of these related search-state arguments into a single struct greatly reduces signature complexity and allows for the removal of the#[allow(clippy::too_many_arguments)]pragma.✅ Verification: I successfully confirmed that the changes did not break the functionality by ensuring that the codebase compiled warning-free with
cargo checkandcargo clippy, formatted correctly usingcargo fmt, and passed all the tests related toworking_setandcodewhale-tui.✨ Result: The signatures are now simpler, easier to read, and
clippyexceptions have been cleanly removed, leaving the project in a slightly healthier state.PR created automatically by Jules for task 18290017424165256247 started by @Hmbown