Unify (mostly) Homebrew code style

[MikeMcQuaid]

Make the Homebrew/cask and Homebrew/homebrew-core style more closely match the rest of Homebrew.

To accomplish this:

• Run brew cask style to ensure we don't break style there when making changes or upgrading RuboCop in Homebrew/brew.
• Fix the HomepageMatchesUrl cop to better handle weird input.
• Remove the now unneeded .rubocop_shared.yml
• Fix the cask fixtures with brew cask style --fix.
• Share more style between Homebrew/brew, casks and formulae.

@Homebrew/cask once I have 👍 from at least one of you'll I'll fix the current style violations in Homebrew/cask (almost 100% brew cask style --fix).

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

---

[MikeMcQuaid]

CC @vitorgalvao as your feedback heavily influenced the approach here and I want you to be OK with this before it ships.

[Rylan12]

Okay, I think I understand this much better now.

Here's my summary:

• brew style: uses Homebrew/.rubocop.yml as its config file
  • Inheritance: Homebrew/.rubocop.yml --> .rubocop_rspec.yml --> .rubocop.yml
• brew style <formula/tap>: uses .rubocop.yml as its config file.
• brew cask style: uses .rubocop_cask.yml as its config file
  • Inheritance: .rubocop_cask.yml --> Homebrew/.rubocop.yml --> .rubocop_rspec.yml --> .rubocop.yml

What's still unclear to me:

If I understand correctly, rubocop looks for a .rubcop.yml file. For formulae, this file is Library/.rubocop.yml. For files in the Library/Homebrew directory, this is Library/Homebrew/.rubocop.yml.

What I don't understand is how casks use Library/.rubocop_cask.yml (aside from when they're checked using brew cask style. The way I understand it, running rubocop on a cask will still use Library/.rubocop.yml as its config file. Doesn't this mean that any additional/overridden cops in Library/.rubocop_cask.yml won't be applied?

Additional questions:

1. Why does .rubocop_cask.yml inherit from Homebrew/.rubocop.yml when .rubocop.yml doesn't? I would think it would inherit directly from .rubocop.yml and make cask-only changes
2. Is .rubocop_rspec.yml only used when inherited from Homebrew/.rubocop.yml? If so, should it be moved to the Homebrew directory?

[vitorgalvao]

I’m fine either way with the indentation, but if we’re making it a fixed number of spaces instead of visual, shouldn’t we also change the first line?

To explain what I mean, on this file we have:

quit:       'my.fancy.package.app',
login_item: 'Fancy',
delete:     [

But given the indentation change, don’t you think it should now be:

quit: 'my.fancy.package.app',
login_item: 'Fancy',
delete: [

[MikeMcQuaid]

But given the indentation change, don’t you think it should now be:

I'm fine with that, it just won't be autocorrected.

[vitorgalvao]

Did a quick check, and it seems sub(/:\s+/, ': ') should be enough to fix these in one swoop. I can do that (and do audit and style checks) after this is live.

[vitorgalvao]

• Fix the HomepageMatchesUrl cop to better handle fixtures.

Ideally we’ll get rid of that check (it should be an audit instead), so no need to spend much time on it. Some progress toward it in #7772.

CC @vitorgalvao as your feedback heavily influenced the approach here and I want you to be OK with this before it ships.

Thank you! But as I mentioned, I agree that the project consistency is more important than my usage. If I could convince you of the advantages of bot single and double quotes—and thus make it the default for everyone—so much the better. But failing that, I’ll get used to only having double quotes. In no way do I want my preference to mean more maintenance work, especially because I’m not the one doing the cop maintenance!

[MikeMcQuaid]

What I don't understand is how casks use Library/.rubocop_cask.yml (aside from when they're checked using brew cask style. The way I understand it, running rubocop on a cask will still use Library/.rubocop.yml as its config file. Doesn't this mean that any additional/overridden cops in Library/.rubocop_cask.yml won't be applied?

Yes. This is why we exclude Casks from those things that are overridden. This is another reason why it would have been nicer to have them be unified.

• Why does .rubocop_cask.yml inherit from Homebrew/.rubocop.yml when .rubocop.yml doesn't? I would think it would inherit directly from .rubocop.yml and make cask-only changes

Homebrew/.rubocop.yml is the Homebrew/brew style, .rubocop.yml is the formula style. The existing cask style its much closer to the prior than the latter.

2. Is .rubocop_rspec.yml only used when inherited from Homebrew/.rubocop.yml? If so, should it be moved to the Homebrew directory?

It's still used for some taps and by brew style.

[MikeMcQuaid]

Thank you! But as I mentioned, I agree that the project consistency is more important than my usage. If I could convince you of the advantages of bot single and double quotes—and thus make it the default for everyone—so much the better. But failing that, I’ll get used to only having double quotes. In no way do I want my preference to mean more maintenance work, especially because I’m not the one doing the cop maintenance!

Don't worry, it's not meaningful extra work to leave the quoting as-is.

[Rylan12]

Thanks for answering all my questions, @MikeMcQuaid! I definitely have a better sense of how this works and why you made the changes you did.

[Rylan12]

Is brew cask style supposed to require single-quotes? Right now it seems to not care whether it's single or double.

Steps to reproduce:

• Checkout this PR
• brew cask edit atom and change some single-quoted strings to double-quoted strings
• brew cask style atom doesn't complain about double quotes
• rubocop --config /usr/local/Homebrew/Library/.rubocop_cask.yml atom.rb also doesn't complain about double quotes

[MikeMcQuaid]

Is brew cask style supposed to require single-quotes? Right now it seems to not care whether it's single or double.

Is that before this PR, with this PR or both?

[Rylan12]

Is that before this PR, with this PR or both?

Only with this PR

[MikeMcQuaid]

Thanks @Rylan12. I dug into this further and the styles that contradict (formulae) and therefore require exclusion are going to be impossible to enforce on casks. You cannot exclude something from a RuboCop rule in one place and then de-exclude it elsewhere, this is just a limitation with RuboCop.

The resulting options are:

1. brew cask style and rubocop neither enforce nor complain about e.g. quote usage.
2. brew cask style enforces a Homebrew/brew-like style (closest to the cask style) but rubocop doesn't enforce/autocorrect deviations from the formula style (e.g. in a user's editor)
3. brew cask style and rubocop enforce a formula-like style (further from the cask style) and rubocop will enforce/autocorrect deviations from this style
4. brew cask style and rubocop enforce a new formula style (e.g. don't force hash-rockets) and rubocop will enforce/autocorrect deviations from this style
   @vitorgalvao and @Homebrew/cask: preferences?

[vitorgalvao]

preferences?

Whichever enforces and is able to autocorrect the most cases.

[MikeMcQuaid]

@vitorgalvao et. al: this has been updated to have a (more) consistent style between Homebrew/brew, casks and formulae. The fixtures are the best place to look here to see the cask style. Once we're 👍 on this approach: I'll mass update homebrew-core and homebrew-cask and push directly to them (without PRs).

[amaiman]

Ah, this would explain why I just got several screens worth of updated casks when I ran brew upgrade. Had me worried for a bit that something could have made a malicious change.

[MikeMcQuaid]

is there a specific reason to use the new when : syntax in 6 casks (deeper/gfortran/maintenance/onyx/openzfs/saoimageds9) when all other casks have the MacOS.version <= : syntax?

I don't understand the question, sorry.

can i just send PRs to bring those in-line with the other ones?

Yes, feel free.

[core-code]

I don't understand the question, sorry.

sorry about being unclear here. those 6 casks now contain when : - no other casks contain this or (afaik) have ever contained this.

Yes, feel free.

thanks, will do