Skip to content

Refactor search.#4253

Merged
reitermarkus merged 16 commits intoHomebrew:masterfrom
reitermarkus:refactor-search
Jun 7, 2018
Merged

Refactor search.#4253
reitermarkus merged 16 commits intoHomebrew:masterfrom
reitermarkus:refactor-search

Conversation

@reitermarkus
Copy link
Copy Markdown
Member

@reitermarkus reitermarkus commented Jun 1, 2018

  • 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?

Refactor search to use CLI::Parser.

@ghost ghost assigned reitermarkus Jun 1, 2018
@ghost ghost added the in progress Maintainers are working on this label Jun 1, 2018
@reitermarkus reitermarkus force-pushed the refactor-search branch 5 times, most recently from 3a9dcef to 25c587f Compare June 1, 2018 17:55
@reitermarkus reitermarkus requested a review from MikeMcQuaid June 1, 2018 20:58
@reitermarkus reitermarkus force-pushed the refactor-search branch 2 times, most recently from aaf9314 to e2d10e3 Compare June 2, 2018 01:09
@reitermarkus reitermarkus requested a review from JCount June 2, 2018 01:38
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is .any? and ! .empty? identical performance wise? I was under the impression the latter was more performant (although that could be a Railism, creeping in)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Seems like it, I didn't know about that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Trying to avoid adding more of these: #4059

Comment thread Library/Homebrew/cli_parser.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could stick each . on a new line if refactoring here anyway?

Comment thread Library/Homebrew/cli_parser.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if/else is a bit more readable here.

Comment thread Library/Homebrew/cli_parser.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Make a block if for a shorter line

Comment thread Library/Homebrew/cli_parser.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

stick the .join on a new line?

Comment thread Library/Homebrew/cmd/search.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

next unless args to shorten the line (and maybe a few variables)

Comment thread Library/Homebrew/cmd/search.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the latter []?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread Library/Homebrew/cmd/search.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unless block form for a shorter line

Comment thread Library/Homebrew/cmd/search.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These feel a bit weird. Maybe return a Hash?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

A Hash cannot be destructed as nicely. Eventually this should be used like this:

remote_formulae, remote_casks = search_taps(query)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@reitermarkus I think given they are arrays a {formulae: [], casks: []} is a nicer interface for the consumer. It's not obvious from the naming that search_taps provides two arrays of arrays as results.

@reitermarkus reitermarkus force-pushed the refactor-search branch 3 times, most recently from 84e1390 to 6d9aa3b Compare June 2, 2018 18:52
Comment thread Library/Homebrew/cli_parser.rb Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this code isn't directly part of this refactor, but may it make sense to make it more closely match the above syntax? Any thoughts @MikeMcQuaid?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@GauthamGoli, do we even need this? We are using OptionParser only to parse arguments and not to display descriptions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@GauthamGoli Given we're not using descriptions anywhere yet I'd be 👍 in deleting the description code until it's used (or using it in some places now)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 For removal if it's not being utilized.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems from #4271 (comment) it will be used very shortly so let's leave it as-is for now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry. I missed this notification. Yeah. I'll be working on this shortly, don't remove it.

@reitermarkus reitermarkus force-pushed the refactor-search branch 2 times, most recently from 0ad6111 to 3f8f1e3 Compare June 2, 2018 21:10
@reitermarkus reitermarkus requested a review from MikeMcQuaid June 6, 2018 15:35
Comment thread Library/Homebrew/dev-cmd/tests.rb Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change this to one line?

@MikeMcQuaid
Copy link
Copy Markdown
Member

Thanks again @reitermarkus! 🚢 when ready.

@reitermarkus reitermarkus force-pushed the refactor-search branch 2 times, most recently from e17e5e3 to bce52e2 Compare June 7, 2018 11:57
@reitermarkus reitermarkus merged commit ce85dd0 into Homebrew:master Jun 7, 2018
@ghost ghost removed the in progress Maintainers are working on this label Jun 7, 2018
@reitermarkus reitermarkus deleted the refactor-search branch June 7, 2018 12:16
@JCount
Copy link
Copy Markdown
Contributor

JCount commented Jun 7, 2018

👍 Good work, @reitermarkus

@lock lock bot added the outdated PR was locked due to age label Jul 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 7, 2018
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.

4 participants