From 32e7268596a0640fc8dbd87263e4420c14757f70 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 26 Feb 2023 13:20:08 +0000 Subject: [PATCH 1/9] rubocop: Only enable `Style/Documentation` for `@api public` code - Suggested in https://github.com/Homebrew/brew/pull/14709#issuecomment-1437461642. - Found the public API paths with `git grep -l "@api public"`. --- Library/.rubocop.yml | 46 ++++++++++++++++++++++++++++++----- Library/Homebrew/.rubocop.yml | 22 ----------------- 2 files changed, 40 insertions(+), 28 deletions(-) diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index 2a5c7350fa..5f0e737be4 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -339,13 +339,47 @@ Style/DisableCopsWithinSourceCodeDirective: - "/**/{Formula,Casks}/*.rb" - "**/{Formula,Casks}/*.rb" -# Don't enforce documentation in casks or formulae. +# Only enforce documentation for public APIs. Style/Documentation: - Exclude: - - "Taps/**/*" - - "/**/{Formula,Casks}/*.rb" - - "**/{Formula,Casks}/*.rb" - - "**/*.rbi" + AllowedConstants: + - Homebrew + Include: + - Homebrew/cask/dsl.rb + - Homebrew/cask/dsl/version.rb + - Homebrew/cask/url.rb + - Homebrew/download_strategy.rb + - Homebrew/formula.rb + - Homebrew/formula_assertions.rb + - Homebrew/formula_free_port.rb + - Homebrew/language/go.rb + - Homebrew/language/java.rb + - Homebrew/language/node.rb + - Homebrew/language/perl.rb + - Homebrew/language/python.rb + - Homebrew/livecheck/strategy/apache.rb + - Homebrew/livecheck/strategy/bitbucket.rb + - Homebrew/livecheck/strategy/cpan.rb + - Homebrew/livecheck/strategy/extract_plist.rb + - Homebrew/livecheck/strategy/git.rb + - Homebrew/livecheck/strategy/github_latest.rb + - Homebrew/livecheck/strategy/gnome.rb + - Homebrew/livecheck/strategy/gnu.rb + - Homebrew/livecheck/strategy/hackage.rb + - Homebrew/livecheck/strategy/json.rb + - Homebrew/livecheck/strategy/launchpad.rb + - Homebrew/livecheck/strategy/npm.rb + - Homebrew/livecheck/strategy/page_match.rb + - Homebrew/livecheck/strategy/pypi.rb + - Homebrew/livecheck/strategy/sourceforge.rb + - Homebrew/livecheck/strategy/sparkle.rb + - Homebrew/livecheck/strategy/xorg.rb + - Homebrew/os.rb + - Homebrew/resource.rb + - Homebrew/utils.rb + - Homebrew/utils/inreplace.rb + - Homebrew/utils/shebang.rb + - Homebrew/utils/string_inreplace_extension.rb + - Homebrew/version.rb Style/DocumentationMethod: Include: diff --git a/Library/Homebrew/.rubocop.yml b/Library/Homebrew/.rubocop.yml index 8f3c4b34a1..a72e17bf66 100644 --- a/Library/Homebrew/.rubocop.yml +++ b/Library/Homebrew/.rubocop.yml @@ -17,28 +17,6 @@ Naming/PredicateName: - is_32_bit? - is_64_bit? -Style/Documentation: - AllowedConstants: - - Homebrew - Exclude: - - "extend/**/*.rb" - - "test/**/*.rb" - - "cask/macos.rb" - - "cli/args.rb" - - "cli/parser.rb" - - "cmd/list.rb" - - "cmd/update-report.rb" - - "dev-cmd/irb.rb" - - "dev-cmd/pr-pull.rb" - - "keg_relocate.rb" - - "os/mac/keg.rb" - - "software_spec.rb" - - "utils.rb" - - "utils/fork.rb" - - "utils/git_repository.rb" - - "utils/popen.rb" - - "utils/shell.rb" - Style/HashAsLastArrayItem: Exclude: - "test/utils/spdx_spec.rb" From 316df75da98678f26a2f2f960d100b35f9fab997 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 26 Feb 2023 14:50:34 +0000 Subject: [PATCH 2/9] utils: Add a comment to `module Kernel` to appease RuboCop - This `Homebrew/utils.rb` file contains one `@api public` method so it's now included in `Style/Documentation`. - This method not having a comment was causing the style specs to fail because this file isn't usually failing RuboCop. - And the test description was confusing so I improved it. --- Library/Homebrew/extend/kernel.rb | 1 + Library/Homebrew/test/style_spec.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/extend/kernel.rb b/Library/Homebrew/extend/kernel.rb index f48cd57ab8..a70637fb25 100644 --- a/Library/Homebrew/extend/kernel.rb +++ b/Library/Homebrew/extend/kernel.rb @@ -1,6 +1,7 @@ # typed: false # frozen_string_literal: true +# Contains shorthand Homebrew utility methods like `ohai`, `opoo`, `odisabled`. module Kernel extend T::Sig diff --git a/Library/Homebrew/test/style_spec.rb b/Library/Homebrew/test/style_spec.rb index 9867bc1e19..227b09b840 100644 --- a/Library/Homebrew/test/style_spec.rb +++ b/Library/Homebrew/test/style_spec.rb @@ -40,7 +40,7 @@ describe Homebrew::Style do describe ".check_style_and_print" do let(:dir) { mktmpdir } - it "returns false for conforming file with only audit-level violations" do + it "returns true (success) for conforming file with only audit-level violations" do # This file is known to use non-rocket hashes and other things that trigger audit, # but not regular, cop violations target_file = HOMEBREW_LIBRARY_PATH/"utils.rb" From 27610e0a66aa205d2820f43191ea23d7876e80c8 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 27 Feb 2023 16:25:56 +0000 Subject: [PATCH 3/9] rubocop: Move `Style/Documentation` stanza - This is better in the `Library/Homebrew/.rubocop.yml` since it's operating on files that are within that directory. --- Library/.rubocop.yml | 42 ----------------------------------- Library/Homebrew/.rubocop.yml | 42 +++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 42 deletions(-) diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index 5f0e737be4..80d2e3c8e1 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -339,48 +339,6 @@ Style/DisableCopsWithinSourceCodeDirective: - "/**/{Formula,Casks}/*.rb" - "**/{Formula,Casks}/*.rb" -# Only enforce documentation for public APIs. -Style/Documentation: - AllowedConstants: - - Homebrew - Include: - - Homebrew/cask/dsl.rb - - Homebrew/cask/dsl/version.rb - - Homebrew/cask/url.rb - - Homebrew/download_strategy.rb - - Homebrew/formula.rb - - Homebrew/formula_assertions.rb - - Homebrew/formula_free_port.rb - - Homebrew/language/go.rb - - Homebrew/language/java.rb - - Homebrew/language/node.rb - - Homebrew/language/perl.rb - - Homebrew/language/python.rb - - Homebrew/livecheck/strategy/apache.rb - - Homebrew/livecheck/strategy/bitbucket.rb - - Homebrew/livecheck/strategy/cpan.rb - - Homebrew/livecheck/strategy/extract_plist.rb - - Homebrew/livecheck/strategy/git.rb - - Homebrew/livecheck/strategy/github_latest.rb - - Homebrew/livecheck/strategy/gnome.rb - - Homebrew/livecheck/strategy/gnu.rb - - Homebrew/livecheck/strategy/hackage.rb - - Homebrew/livecheck/strategy/json.rb - - Homebrew/livecheck/strategy/launchpad.rb - - Homebrew/livecheck/strategy/npm.rb - - Homebrew/livecheck/strategy/page_match.rb - - Homebrew/livecheck/strategy/pypi.rb - - Homebrew/livecheck/strategy/sourceforge.rb - - Homebrew/livecheck/strategy/sparkle.rb - - Homebrew/livecheck/strategy/xorg.rb - - Homebrew/os.rb - - Homebrew/resource.rb - - Homebrew/utils.rb - - Homebrew/utils/inreplace.rb - - Homebrew/utils/shebang.rb - - Homebrew/utils/string_inreplace_extension.rb - - Homebrew/version.rb - Style/DocumentationMethod: Include: - "Homebrew/formula.rb" diff --git a/Library/Homebrew/.rubocop.yml b/Library/Homebrew/.rubocop.yml index a72e17bf66..5ec6504655 100644 --- a/Library/Homebrew/.rubocop.yml +++ b/Library/Homebrew/.rubocop.yml @@ -17,6 +17,48 @@ Naming/PredicateName: - is_32_bit? - is_64_bit? +# Only enforce documentation for public APIs. +Style/Documentation: + AllowedConstants: + - Homebrew + Include: + - cask/dsl.rb + - cask/dsl/version.rb + - cask/url.rb + - download_strategy.rb + - formula.rb + - formula_assertions.rb + - formula_free_port.rb + - language/go.rb + - language/java.rb + - language/node.rb + - language/perl.rb + - language/python.rb + - livecheck/strategy/apache.rb + - livecheck/strategy/bitbucket.rb + - livecheck/strategy/cpan.rb + - livecheck/strategy/extract_plist.rb + - livecheck/strategy/git.rb + - livecheck/strategy/github_latest.rb + - livecheck/strategy/gnome.rb + - livecheck/strategy/gnu.rb + - livecheck/strategy/hackage.rb + - livecheck/strategy/json.rb + - livecheck/strategy/launchpad.rb + - livecheck/strategy/npm.rb + - livecheck/strategy/page_match.rb + - livecheck/strategy/pypi.rb + - livecheck/strategy/sourceforge.rb + - livecheck/strategy/sparkle.rb + - livecheck/strategy/xorg.rb + - os.rb + - resource.rb + - utils.rb + - utils/inreplace.rb + - utils/shebang.rb + - utils/string_inreplace_extension.rb + - version.rb + Style/HashAsLastArrayItem: Exclude: - "test/utils/spdx_spec.rb" From 9ec74fea2ae9195b690f53f4c0ce5706ebefdfcf Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 27 Feb 2023 23:10:18 +0000 Subject: [PATCH 4/9] rubocop: Reinstate taps ignore for `Style/Documentation` --- Library/.rubocop.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index 80d2e3c8e1..fc89c85ca0 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -339,6 +339,14 @@ Style/DisableCopsWithinSourceCodeDirective: - "/**/{Formula,Casks}/*.rb" - "**/{Formula,Casks}/*.rb" +# The files actually scanned in this cop are in `Library/Homebrew/.rubocop.yml`. +Style/Documentation: + Exclude: + - "Taps/**/*" + - "/**/{Formula,Casks}/*.rb" + - "**/{Formula,Casks}/*.rb" + - "**/*.rbi" + Style/DocumentationMethod: Include: - "Homebrew/formula.rb" From 93e2f86cf86805f3a1ebf9b63b11378a2b32e02c Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 27 Feb 2023 23:38:34 +0000 Subject: [PATCH 5/9] ci: Add an Action to check RuboCop filepaths - This will stop the `Style/Documentation` filepath includes getting out of sync with what we declare as a public API, thus ensuring that everything is documented. - Maybe we could also add a job here to check that _all_ the paths in the RuboCop config still exist, but that's for another time. --- .github/workflows/rubocop-filepaths.yml | 29 +++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 .github/workflows/rubocop-filepaths.yml diff --git a/.github/workflows/rubocop-filepaths.yml b/.github/workflows/rubocop-filepaths.yml new file mode 100644 index 0000000000..0783f2fcda --- /dev/null +++ b/.github/workflows/rubocop-filepaths.yml @@ -0,0 +1,29 @@ +name: RuboCop filepaths + +on: + push: + branches: + - master + pull_request: + +jobs: + check: + runs-on: ubuntu-22.04 + if: github.repository == 'Homebrew/brew' + steps: + - name: Checkout repo + uses: actions/checkout@v3 + + - name: Check public API docs + working-directory: Library/Homebrew + run: | + public_apis=$(git grep -l "@api public" | wc -l | tr -d ' ') + rubocop_docs=$(yq '.Style/Documentation.Include' .rubocop.yml | wc -l | tr -d ' ') + + if [[ public_apis -ne rubocop_docs ]] + then + echo "All public Homebrew APIs should be included in the Style/Documentation RuboCop." + echo "There were ${public_apis} '@api public' lines but ${rubocop_docs} filepaths for the 'Style/Documentation' RuboCop." + echo "Add or remove the filepaths from Library/Homebrew/.rubocop.yml as appropriate." + exit 1 + fi From 033dacac103058557641ee14a3a924a640673afc Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 28 Feb 2023 00:19:53 +0000 Subject: [PATCH 6/9] rubocop: Tweak `Style/Documentation` include paths after PR 14805 - The `Kernel` module, which needed a comment because it has an "@api public" method in it, moved. --- Library/Homebrew/.rubocop.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/.rubocop.yml b/Library/Homebrew/.rubocop.yml index 5ec6504655..06f2295cdf 100644 --- a/Library/Homebrew/.rubocop.yml +++ b/Library/Homebrew/.rubocop.yml @@ -26,6 +26,7 @@ Style/Documentation: - cask/dsl/version.rb - cask/url.rb - download_strategy.rb + - extend/kernel.rb - formula.rb - formula_assertions.rb - formula_free_port.rb @@ -53,7 +54,6 @@ Style/Documentation: - livecheck/strategy/xorg.rb - os.rb - resource.rb - - utils.rb - utils/inreplace.rb - utils/shebang.rb - utils/string_inreplace_extension.rb From 267d72a9d02ad1b6cf263c8f8ae1fa4b7eb545b3 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 28 Feb 2023 12:50:05 +0000 Subject: [PATCH 7/9] More comments and TODOs Co-authored-by: Mike McQuaid --- Library/Homebrew/.rubocop.yml | 1 + Library/Homebrew/extend/kernel.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/Library/Homebrew/.rubocop.yml b/Library/Homebrew/.rubocop.yml index 06f2295cdf..b0c21afab7 100644 --- a/Library/Homebrew/.rubocop.yml +++ b/Library/Homebrew/.rubocop.yml @@ -18,6 +18,7 @@ Naming/PredicateName: - is_64_bit? # Only enforce documentation for public APIs. +# Checked by the tests.yml syntax job Style/Documentation: AllowedConstants: - Homebrew diff --git a/Library/Homebrew/extend/kernel.rb b/Library/Homebrew/extend/kernel.rb index a70637fb25..1cf47c04d7 100644 --- a/Library/Homebrew/extend/kernel.rb +++ b/Library/Homebrew/extend/kernel.rb @@ -2,6 +2,7 @@ # frozen_string_literal: true # Contains shorthand Homebrew utility methods like `ohai`, `opoo`, `odisabled`. +# TODO: move these out of `Kernel`. module Kernel extend T::Sig From f7dd63008ee195c471042a32251ec0e3bc91571e Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 28 Feb 2023 12:52:21 +0000 Subject: [PATCH 8/9] ci: Include RuboCop filepaths check in `syntax` workflow - The slowest part of the separate workflow was the repo checkout step, so include it in here to avoid the overhead. --- .github/workflows/rubocop-filepaths.yml | 29 ------------------------- .github/workflows/tests.yml | 13 +++++++++++ 2 files changed, 13 insertions(+), 29 deletions(-) delete mode 100644 .github/workflows/rubocop-filepaths.yml diff --git a/.github/workflows/rubocop-filepaths.yml b/.github/workflows/rubocop-filepaths.yml deleted file mode 100644 index 0783f2fcda..0000000000 --- a/.github/workflows/rubocop-filepaths.yml +++ /dev/null @@ -1,29 +0,0 @@ -name: RuboCop filepaths - -on: - push: - branches: - - master - pull_request: - -jobs: - check: - runs-on: ubuntu-22.04 - if: github.repository == 'Homebrew/brew' - steps: - - name: Checkout repo - uses: actions/checkout@v3 - - - name: Check public API docs - working-directory: Library/Homebrew - run: | - public_apis=$(git grep -l "@api public" | wc -l | tr -d ' ') - rubocop_docs=$(yq '.Style/Documentation.Include' .rubocop.yml | wc -l | tr -d ' ') - - if [[ public_apis -ne rubocop_docs ]] - then - echo "All public Homebrew APIs should be included in the Style/Documentation RuboCop." - echo "There were ${public_apis} '@api public' lines but ${rubocop_docs} filepaths for the 'Style/Documentation' RuboCop." - echo "Add or remove the filepaths from Library/Homebrew/.rubocop.yml as appropriate." - exit 1 - fi diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 5aa53cfe54..b8501d2ac4 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -46,6 +46,19 @@ jobs: - run: brew typecheck + - name: Check RuboCop filepaths + working-directory: Library/Homebrew + run: | + public_apis=$(git grep -l "@api public" | wc -l | tr -d ' ') + rubocop_docs=$(yq '.Style/Documentation.Include' .rubocop.yml | wc -l | tr -d ' ') + if [[ public_apis -ne rubocop_docs ]] + then + echo "All public Homebrew APIs should be included in the Style/Documentation RuboCop." + echo "There were ${public_apis} '@api public' lines but ${rubocop_docs} filepaths for the 'Style/Documentation' RuboCop." + echo "Add or remove the filepaths from Library/Homebrew/.rubocop.yml as appropriate." + exit 1 + fi + tap-syntax: name: tap syntax needs: syntax From e82869b3ac7982301fda06a5093ed087771bce9e Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 28 Feb 2023 13:30:19 +0000 Subject: [PATCH 9/9] ci: Fix working directory for RuboCop filepath step? --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index b8501d2ac4..8d65a0c650 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -47,7 +47,7 @@ jobs: - run: brew typecheck - name: Check RuboCop filepaths - working-directory: Library/Homebrew + working-directory: ${{ steps.set-up-homebrew.outputs.repository-path }}/Library/Homebrew run: | public_apis=$(git grep -l "@api public" | wc -l | tr -d ' ') rubocop_docs=$(yq '.Style/Documentation.Include' .rubocop.yml | wc -l | tr -d ' ')