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

Fix brew audit Formula/formula.rb #8589

Merged
merged 2 commits into from Nov 16, 2020
Merged

Fix brew audit Formula/formula.rb #8589

merged 2 commits into from Nov 16, 2020

Conversation

claui
Copy link
Contributor

@claui claui commented Sep 3, 2020

  • 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?
    Reports the presence of # rubocop:disable as a violation in 70 cases.
    Update: A couple of style checks fail, see below.
    Update 2: All style checks pass now for me.
  • Have you successfully run brew tests with your changes locally?
    One test (style_spec.rb) fails for me but I feel it should pass, hence WIP.
    Update: A couple of tests fail on my local machine but they’re all unrelated.

Explanation

Fix a regression introduced in PR #8542, which wouldn’t exclude formulae and casks from stricter style checks properly unless tapped. This caused brew audit Formula/formula.rb to report violations which were not meant for formulae and casks.

The fix is to add Exclude patterns for formulae and casks in any git cloned tap’s working tree.

Working outside of the productive Homebrew installation makes sure that the latter doesn’t interfere with development, and vice versa. It also helps track work in progress, especially if one tends to forget things.

WIP because style_spec.rb inexplicably fails for me.
Update: passes now.

Failing style checks

WIP because the following style checks still fail:

Offenses:

/usr/local/Homebrew/Library/Homebrew/load_path.rb:7:1: C: Missing top-level module documentation comment.
module LoadPath
^^^^^^
/usr/local/Homebrew/Library/Homebrew/load_path.rb:9:5: C: Add empty line after guard clause.
    return if $LOAD_PATH.include?(pathname.to_s)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/usr/local/Homebrew/Library/Homebrew/tap.rb:390:5: C: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
    if lib_dir?
    ^^
/usr/local/Homebrew/Library/Homebrew/tap.rb:391:19: C: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
      contents << 'custom code library'
                  ^^^^^^^^^^^^^^^^^^^^^
/usr/local/Homebrew/Library/Homebrew/test/support/fixtures/cask/Casks/with-conditional-caveats.rb:12:48: C: Comment to disable/enable RuboCop.
    puts "This caveat is conditional" if false # rubocop:disable Lint/LiteralAsCondition

Library/.rubocop.yml Outdated Show resolved Hide resolved
Library/.rubocop.yml Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

brew style is fixed: https://github.com/Homebrew/brew/pull/8589/checks?check_run_id=1076020647#step:10:8

@claui claui changed the title WIP: Fix brew audit Formula/formula.rb Fix brew audit Formula/formula.rb Sep 13, 2020
@claui
Copy link
Contributor Author

claui commented Sep 13, 2020

Only one style failure remains:

Library/Homebrew/test/support/fixtures/cask/Casks/with-conditional-caveats.rb:12:48: C: >Style/DisableCopsWithinSourceCodeDirective: Comment to disable/enable RuboCop.
   puts "This caveat is conditional" if false # rubocop:disable Lint/LiteralAsCondition
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Looks like we also have to exclude 'Homebrew/test/**/Casks/**/*.rb', just like we do it in the Style/FrozenStringLiteralComment cop.

@reitermarkus
Copy link
Member

reitermarkus commented Sep 13, 2020

Or you can add a dummy conditional like MacOS.version >= :catalina instead of false since it's only a single file.

@claui
Copy link
Contributor Author

claui commented Sep 13, 2020

Or you can add a dummy conditional like MacOS.version >= :catalina instead of false.

Clever idea – however, what do we do if future me (say, when catalina falls out of support) stumbles upon that clause and spends an afternoon figuring out why it‘s there? 🤔

@reitermarkus
Copy link
Member

Make it MacOS.version == MacOS.version so it will never fail then. 😉

@claui
Copy link
Contributor Author

claui commented Sep 13, 2020

Make it MacOS.version == MacOS.version so it will never fail then. 😉

If I’m not back by midnight, you can probably find me in jail.

@claui
Copy link
Contributor Author

claui commented Sep 13, 2020

@reitermarkus The cops say Binary operator == has identical operands but I’ve been able to trick them:

puts "This caveat is conditional" unless String("Caffeine") == "Caffeine"

@MikeMcQuaid
Copy link
Member

Failing on linuxbrew-core (which means the way these rules are being applied to formula has changed): https://github.com/Homebrew/brew/pull/8589/checks?check_run_id=1109379026#step:16:1

@reitermarkus
Copy link
Member

@SeekingMeaning, @claui mentioned (in a commit comment which I can't find anymore) that ** does not exclude files outside of HOMEBREW_LIBRARY_PATH but /** does.

@claui
Copy link
Contributor Author

claui commented Sep 18, 2020

@SeekingMeaning, @claui mentioned (in a commit comment which I can't find anymore) that ** does not exclude files outside of HOMEBREW_LIBRARY_PATH but /** does.

Here’s the source: https://docs.rubocop.org/rubocop/configuration.html#path-relativity

@reitermarkus
Copy link
Member

So maybe {,/}** works?

@MikeMcQuaid
Copy link
Member

@claui Any news here? Would be good to get this merged.

@MikeMcQuaid
Copy link
Member

@claui Could you try to finish this off or close it out? Thanks.

@claui
Copy link
Contributor Author

claui commented Nov 11, 2020

@SeekingMeaning What purpose did your commit intend to serve? From my point of view, it simply re-introduces the very bug that this PR is trying to fix. Do you want to fix it or shall I revert?

@SeekingMeaning
Copy link
Contributor

I can't remember off the top of my head, but feel free to revert

Fix a regression introduced in PR #8542, which wouldn’t exclude
formulae and casks from stricter style checks properly unless tapped.
This caused `brew audit Formula/formula.rb` to report violations which
were not meant for formulae and casks.

The fix is to add Exclude patterns for formulae and casks in any
`git clone`d tap’s working tree.

Working outside of the productive Homebrew installation makes sure that
the latter doesn’t interfere with development, and vice versa.
It also helps track work in progress, especially if one tends to forget
things.
@claui
Copy link
Contributor Author

claui commented Nov 16, 2020

Thanks @reitermarkus for reinstating the previous state.
I hope I can manage to fix the remaining broken tests now.

@reitermarkus
Copy link
Member

Everything seems to be working now.

@reitermarkus reitermarkus merged commit 218d3e2 into Homebrew:master Nov 16, 2020
@claui claui deleted the fix-audit-for-files branch November 16, 2020 20:30
@claui
Copy link
Contributor Author

claui commented Nov 16, 2020

Thank you ❤️

@MikeMcQuaid
Copy link
Member

Thanks @claui for starting this and @reitermarkus for finishing this off.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 18, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 18, 2020
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.

None yet

5 participants