Skip to content
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

Requirement tweaks #487

Merged
merged 5 commits into from
Jul 11, 2016
Merged

Requirement tweaks #487

merged 5 commits into from
Jul 11, 2016

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jul 10, 2016

  • 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 tests with your changes locally?

More details in commit messages but: fix up tuntap and osxfuse requirements so Homebrew/homebrew-core#2075 and Homebrew/homebrew-core#2076 can be merged and tweak the requirements messaging based on the local testing of these.

CC @DomT4 @AnastasiaSulyagina

@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Jul 10, 2016
@@ -31,7 +31,7 @@ def option_names

# The message to show when the requirement is not met.
def message
s = ""
s = "#{self.class} unsatisfied!\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably a non-issue (or an irrelevant corner case), as most in-formula requirements tend to provide their own message method, but if that wasn't the case, this would result in pretty ugly output like:

$ brew install slimerjs
slimerjs: Formulary::FormulaNamespace4237eb49a894bb8c7e324716724ef7cb::FirefoxRequirement unsatisfied!

You can install with Homebrew Cask:
  brew cask install firefox
Error: An unsatisfied requirement failed this build.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will strip everything up to and including the ::

@UniqMartin
Copy link
Contributor

Two minor remarks, but otherwise 👍.

@DomT4
Copy link
Member

DomT4 commented Jul 10, 2016

Agree with @UniqMartin's points. I think it's safe to completely remove NonBinaryOsxfuseRequirement and presumably the related Dependency => Requirement logic from compat/requirements? Otherwise, 👍.

@MikeMcQuaid
Copy link
Member Author

Agree with @UniqMartin's points. I think it's safe to completely remove NonBinaryOsxfuseRequirement and presumably the related Dependency => Requirement logic from compat/requirements? Otherwise, 👍.

Can't yet as the formula relies on it and we'll need to merge this before the formula change (and can remove afters)

Make it more obvious which class was unsatisfied to produce this error
message.
Previously this was only using the last line.
Nicer to split this onto two lines.
We’re just supporting the Cask now.
We’re just supporting the Cask now.
@MikeMcQuaid MikeMcQuaid merged commit 4a73249 into Homebrew:master Jul 11, 2016
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Jul 11, 2016
@MikeMcQuaid MikeMcQuaid deleted the requirement-tweaks branch July 11, 2016 12:14
@DomT4
Copy link
Member

DomT4 commented Jul 11, 2016

Can't yet as the formula relies on it and we'll need to merge this before the formula change (and can remove afters)

Bleh. Yeah, forgot this. 👍.

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants