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

various formulae: replace compound unless with if #68527

Conversation

SeekingMeaning
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

This is part of Homebrew/brew#10256, which adds a rubocop that disallows unless statements with multiple conditions

Truth table for reference:

p q !(p && q) !p || !q !(p || q) !p && !q
FFTTTT
FTTTFF
TFTTFF
TTFFFF

@carlocab
Copy link
Member

carlocab commented Jan 7, 2021

I don't mind getting rid of unlesss, but simultaneously making the substitution

!(p && q) => (!p) || (!q)

is, I think, a regression for readability. I'd prefer you just stuck with the !(p && q) form. Or does rubocop complain about those too?

@SeekingMeaning
Copy link
Contributor Author

Yes, it doesn't like if !condition

@SeekingMeaning
Copy link
Contributor Author

An alternative is to create a variable and then do unless variable

@carlocab
Copy link
Member

carlocab commented Jan 8, 2021

Nah, that's annoying too. Your changes are fine then.

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants