New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup search module #14260
Cleanup search module #14260
Conversation
This doesn't need to be isolated anymore because there is a check for the formula and cask args and formula is passed by default on Linux now.
- Move cask logic back into method from extend/os and check whether to display it based on args since formula? is passed by default on Linux now - Use #search_descriptions in `brew desc` instead of duplicating logic - Remove need for extend/os/search
- Instead use class methods. - This is better than use it as a mixin when only a small number of methods are used in each class or module. - It also allows us to conditionally require it in `brew install`. - Removed unused search require in descriptions.rb.
Review period will end on 2022-12-20 at 00:00:00 UTC. |
@@ -3,15 +3,12 @@ | |||
|
|||
require "formula" | |||
require "formula_versions" | |||
require "search" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't seem to be getting used at all in here.
Now I'm wondering if a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup!
Yeh: if we're using |
Review period ended. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This is a cleanup of the search related logic that is related to #14240.
Essentially, it does a few things.
extend/os
since the change to passing the--formula
everywhere.cmd/desc.rb
andsearch.rb
so I changed it so both usesearch.rb
.extend/os
for the reason given above andextend/os/search
has now been removed.search.rb
into class methods and stops extending/including it. This is mostly a style thing. It has the advantages of not polluting the local namespace, allowing us to conditionally include it, and also makes it easier to see which methods are external and internal to a file.Of the above three changes I think the first two are no-brainers while I could be convinced that the third one is unnecessary since it's mostly a style thing.
To test for regressions I tested the following commands along with their
--cask
and--formula
counterparts.Results
Results
Results