Skip to content

search: add separate flag for formulae#7094

Merged
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
vidusheeamoli:search-add-formulae-flag
Mar 4, 2020
Merged

search: add separate flag for formulae#7094
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
vidusheeamoli:search-add-formulae-flag

Conversation

@vidusheeamoli
Copy link
Copy Markdown
Contributor

@vidusheeamoli vidusheeamoli commented Feb 29, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This PR attempts to solve issue #6975.

Right now, this PR has a lot of scope for improvement, but I am determined to get this done right and merged. As discussed in the above issue I have added a separate --formulae flag in brew search and this is what brew search looks like now:

brew search					#lists both formulae and casks
brew search --formulae		#lists only formulae
brew search --casks 		#lists only casks

I have tested these changes locally, and they seem to work so far.

In this version of implementation, brew search first lists all cask names(sorted) followed by sorted formula names. Is this correct, or should I mix all cask and formula names in a list and then print this list in a sorted manner (this would mix all cask and formula names).

Thank you, @issyl0 for telling me such a convenient way to test out my changes locally -- it saved me a lot of time! Please take a look and let me know. 😃

Thanks! 🚀

@MikeMcQuaid
Copy link
Copy Markdown
Member

Great work here @vidusheeamoli, you're off to a fantastic start! I think it'd be great to get this PR to a point where brew search --formulae chrome works. I'm not sure that having just brew search by default list casks is super useful (and it may break other tools) but having the brew search --formulae output just formulae does feel appropriate.

Additionally, CI is failing here so when you make your next changes run brew man and commit the changes there.

Thanks again!

@vidusheeamoli
Copy link
Copy Markdown
Contributor Author

vidusheeamoli commented Mar 2, 2020

Thank you for the feedback @MikeMcQuaid! 😃

I'm not sure that having just brew search by default list casks is super useful (and it may break other tools) but having the brew search --formulae output just formulae does feel appropriate.

Forgive me for the silly question, but does this mean that brew search and brew search --formulae will both list only formulae? Wouldn't it be confusing for users to have 2 different commands do the exact same job?

Edit: On thinking about it, i've realised that this implementation will give more clarity as compared to the current scenario. brew search -- by default will list only formulae, but having the --formulae flag also provides more consistency to the users (since there is already a flag for casks). Hope I'm thinking in the right direction here.

Additionally, CI is failing here so when you make your next changes run brew man and commit the changes there.

Sure thing! I will also be adding tests for my changes. :)

@MikeMcQuaid
Copy link
Copy Markdown
Member

Forgive me for the silly question, but does this mean that brew search and brew search --formulae will both list only formulae?

Yep!

Wouldn't it be confusing for users to have 2 different commands do the exact same job?

Perhaps but I think if --formulae does other things (e.g. when passed arguments) then it makes sense to be consistent.

On thinking about it, i've realised that this implementation will give more clarity as compared to the current scenario. brew search -- by default will list only formulae, but having the --formulae flag also provides more consistency to the users (since there is already a flag for casks). Hope I'm thinking in the right direction here.

Exactly!

Sure thing! I will also be adding tests for my changes. :)

Cool! Tests aren't required but if you do add them don't make more :integration tests as they are slow (so we keep the numbers down).

@vidusheeamoli vidusheeamoli force-pushed the search-add-formulae-flag branch 2 times, most recently from a500508 to a3d81be Compare March 2, 2020 17:39
@vidusheeamoli
Copy link
Copy Markdown
Contributor Author

vidusheeamoli commented Mar 2, 2020

Hello @MikeMcQuaid!

I've incorporated all the discussed changes; and here is what brew search looks like now:

brew search							#default, only lists formulae
brew search --formulae				#lists only formulae
brew search --casks 				#lists only casks
brew search <arg>					#lists both formulae and casks
brew search --formulae <arg>		#lists only formulae
brew search --casks <arg>			#lists only casks

Edit: Here is a screenshot for you:
Screen Shot 2020-03-02 at 10 06 13 PM

I've also decided to add tests in a separate PR (if that's okay with you?) and I'll be working on it soon. The changes have been tested locally on my system as well.

Please let me know if you have any questions regarding the edge cases.

Looking forward to your feedback!

Thank you. 😃

@MikeMcQuaid
Copy link
Copy Markdown
Member

brew search #lists both formulae and casks

I think this is the bit that should only list formulae. Otherwise: sounds good!

Comment thread Library/Homebrew/cmd/search.rb Outdated
Comment thread Library/Homebrew/cmd/search.rb Outdated
@vidusheeamoli
Copy link
Copy Markdown
Contributor Author

vidusheeamoli commented Mar 2, 2020

I think this is the bit that should only list formulae. Otherwise: sounds good!

Oh, this is already incorporated, only the comment is wrong. 😛

Also, addressing rest of the reviews right now. Thank you @MikeMcQuaid ! 😄

@vidusheeamoli vidusheeamoli force-pushed the search-add-formulae-flag branch from a3d81be to 10ba0d5 Compare March 2, 2020 19:21
@vidusheeamoli vidusheeamoli reopened this Mar 2, 2020
@vidusheeamoli vidusheeamoli force-pushed the search-add-formulae-flag branch from 49ec84d to 4ae2bed Compare March 2, 2020 19:55
@vidusheeamoli
Copy link
Copy Markdown
Contributor Author

Sorry for the delay, a brew style test had failed earlier. Thank you for your patience @MikeMcQuaid! 😃

Comment thread Library/Homebrew/cmd/search.rb Outdated
Comment thread Library/Homebrew/cmd/search.rb Outdated
Comment thread Library/Homebrew/cmd/search.rb Outdated
Comment thread Library/Homebrew/cmd/search.rb Outdated
@vidusheeamoli vidusheeamoli force-pushed the search-add-formulae-flag branch from 5c6e1ca to 9aa6142 Compare March 3, 2020 17:48
Comment thread Library/Homebrew/cmd/search.rb Outdated
Comment thread Library/Homebrew/cmd/search.rb Outdated
Comment thread Library/Homebrew/cmd/search.rb Outdated
Comment thread Library/Homebrew/cmd/search.rb Outdated
@vidusheeamoli vidusheeamoli force-pushed the search-add-formulae-flag branch from 9aa6142 to 8bdb82c Compare March 4, 2020 11:42
@vidusheeamoli vidusheeamoli force-pushed the search-add-formulae-flag branch from 8bdb82c to 67d012a Compare March 4, 2020 12:28
Copy link
Copy Markdown
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @vidusheeamoli!

@MikeMcQuaid MikeMcQuaid merged commit c42d43c into Homebrew:master Mar 4, 2020
@EricFromCanada
Copy link
Copy Markdown
Member

Resolves #6761.

@vidusheeamoli vidusheeamoli deleted the search-add-formulae-flag branch March 5, 2020 07:15
@lock lock bot added the outdated PR was locked due to age label Apr 4, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants