brew-rs: support some flags for list and search command#21844
brew-rs: support some flags for list and search command#21844
list and search command#21844Conversation
There was a problem hiding this comment.
Pull request overview
Adds limited flag support to the Rust (brew-rs) implementations of brew search and brew list, enabling native handling of --formula/--formulae and --cask/--casks in more cases rather than delegating to the Ruby backend.
Changes:
- Parse
--formula/--caskflags inbrew-rs searchand filter which API name sets are searched. - Parse
--formula/--caskflags inbrew-rs listand filter which installed sets are listed / resolved. - Adjust “not found” messaging to reflect the selected search/list scope.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
Library/Homebrew/rust/brew-rs/src/commands/search.rs |
Adds arg parsing for --formula/--cask and conditionally searches formula and/or cask name lists. |
Library/Homebrew/rust/brew-rs/src/commands/list.rs |
Adds arg parsing for --formula/--cask and conditionally lists installed formulae/casks or resolves paths for named packages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| missing.push(name.clone()); | ||
| missing.push(name); | ||
| } |
There was a problem hiding this comment.
missing.push(name) here pushes an &&str (because name comes from iterating over &parsed_args.packages). This works but leads to a nested reference type inferred for missing, which is easy to trip over in future edits. Consider iterating as for &name in ... (or pushing *name) so missing is a Vec<&str>.
| pub fn run(args: &[String]) -> BrewResult<ExitCode> { | ||
| if args.len() != 2 || args[1].starts_with('-') { | ||
| return delegate::run(args); | ||
| } | ||
| let parsed_args = { | ||
| let mut result = Args::default(); | ||
| let mut args_iter = args.iter(); | ||
| args_iter.next(); // Skip `search` | ||
| for arg in args_iter { | ||
| match arg.as_str() { | ||
| "--formula" | "--formulae" => result.formula = true, | ||
| "--cask" | "--casks" => result.cask = true, | ||
| s if !s.starts_with('-') && result.package_name.is_none() => { | ||
| result.package_name = Some(s) | ||
| } | ||
| _ => return delegate::run(args), | ||
| } | ||
| } | ||
| if !result.formula && !result.cask { | ||
| result.formula = true; | ||
| result.cask = true; | ||
| } | ||
| result | ||
| }; |
There was a problem hiding this comment.
New flag-handling behavior (--formula/--cask filtering + the “both flags” case) isn’t covered by tests in this crate. Since tests/cli.rs already asserts behavior for search, please add coverage for brew-rs search --formula ..., --cask ..., and --formula --cask ... (especially if matching Ruby precedence).
| pub fn run(args: &[String]) -> BrewResult<ExitCode> { | ||
| if args[1..] | ||
| .iter() | ||
| .any(|arg| arg.starts_with('-') || arg.contains('/')) | ||
| { | ||
| return delegate::run(args); | ||
| } | ||
| let parsed_args = { | ||
| let mut result = Args::default(); | ||
| let mut args_iter = args.iter(); | ||
| args_iter.next(); // Skip `list` | ||
| for arg in args_iter { | ||
| match arg.as_str() { | ||
| "--formula" | "--formulae" => result.formula = true, | ||
| "--cask" | "--casks" => result.cask = true, | ||
| s if !s.starts_with('-') && !s.contains('/') => result.packages.push(s), | ||
| _ => return delegate::run(args), | ||
| } | ||
| } | ||
| if !result.formula && !result.cask { | ||
| result.formula = true; | ||
| result.cask = true; | ||
| } | ||
| result | ||
| }; |
There was a problem hiding this comment.
New flag-handling behavior for list (including --formula/--cask filtering and the conflict case) isn’t covered by tests in this crate. Since tests/cli.rs already has list parity tests, add cases for brew-rs list --formula, --cask, and --formula --cask to lock in intended behavior and prevent parity regressions.
| let formulae_or_casks = if parsed_args.formula && parsed_args.cask { | ||
| "formulae or casks" | ||
| } else if parsed_args.formula { | ||
| "formulae" | ||
| } else { | ||
| "casks" | ||
| }; |
There was a problem hiding this comment.
When both --formula and --cask are specified, the Ruby implementation effectively searches formulae only (because Search.search_names checks args.formula? before args.cask?, see Library/Homebrew/search.rb). The Rust implementation treats both flags as “search both”, which can diverge from brew search output. To preserve parity, match Ruby’s precedence (or delegate when both are present).
botantony
force-pushed
the
brew-rs-support-some-flags
branch
from
2b49abd to
061290a
Compare
Signed-off-by: botantony <antonsm21@gmail.com>
botantony
force-pushed
the
brew-rs-support-some-flags
branch
from
061290a to
c41f5ed
Compare
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Let's hold off adding any flags for these commands please for a few reasons:
- we still haven't decided even which commands we're definitely going to defer to the Rust frontend vs. Ruby one
- we haven't decided what flags we're going to add
- the flags we add will end up sticking around
- as-is from reading this code it's nearly impossible to identify what flags we support/don't compared to the DSL approach in Ruby. I want to ensure we make any flags we do add are obvious from glancing at the first few lines of the file.
brew lgtm(style, typechecking and tests) with your changes locally?