Skip to content

Refactor brew cask search.#4306

Merged
reitermarkus merged 3 commits intoHomebrew:masterfrom
reitermarkus:cask-search
Jun 8, 2018
Merged

Refactor brew cask search.#4306
reitermarkus merged 3 commits intoHomebrew:masterfrom
reitermarkus:cask-search

Conversation

@reitermarkus
Copy link
Copy Markdown
Member

@reitermarkus reitermarkus commented Jun 7, 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 brew cask search once more before merging it into brew search.

@ghost ghost assigned reitermarkus Jun 7, 2018
@ghost ghost added the in progress Maintainers are working on this label Jun 7, 2018
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 this needed for? It'd be nice to instead move logic that's needed for a generalised brew integration test out of cask/lib.

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.

The generic stuff is already outside of cask/lib, but we cannot get around having access to the Cask class.

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.

Ok. Can you see what overhead it adds to load this code (if any) on every brew process and brew tests integration test? Thanks!

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.

It doesn't load code, it only adds this path to the $LOAD_PATH.

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.

My bad, 👍

@MikeMcQuaid
Copy link
Copy Markdown
Member

:shipit:

@reitermarkus reitermarkus merged commit 61bcec4 into Homebrew:master Jun 8, 2018
@ghost ghost removed the in progress Maintainers are working on this label Jun 8, 2018
@reitermarkus reitermarkus deleted the cask-search branch June 8, 2018 12:18
@lock lock bot added the outdated PR was locked due to age label Jul 8, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 8, 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.

2 participants