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

a*: move to a/ subdirectory. #138921

Merged
merged 1 commit into from
Aug 10, 2023
Merged

a*: move to a/ subdirectory. #138921

merged 1 commit into from
Aug 10, 2023

Conversation

MikeMcQuaid
Copy link
Member

Continuing the sharding work now that ack.rb was moved without issue.

@github-actions github-actions bot added the automerge-skip `brew pr-automerge` will skip this pull request label Aug 7, 2023
@MikeMcQuaid MikeMcQuaid added the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label Aug 7, 2023
@carlocab
Copy link
Member

carlocab commented Aug 7, 2023

Aliases need updating. The code for tracking style exceptions also needs to be modified to take sharding into account. It seems to me that a lot of this would be simplified if we had code for finding the repository root given a file.

@MikeMcQuaid
Copy link
Member Author

MikeMcQuaid commented Aug 7, 2023

Aliases need updating. The code for tracking style exceptions also needs to be modified to take sharding into account.

@carlocab Will do these, thanks 👍🏻.

It seems to me that a lot of this would be simplified if we had code for finding the repository root given a file.

Can you elaborate a bit on that? I think we might do.

@carlocab
Copy link
Member

carlocab commented Aug 7, 2023

Can you elaborate a bit on that? I think we might do.

This is just following-up from my observation at Homebrew/brew#15740 (comment). It seems like there are a few places where we have a path but we want to know the tap repository it came from.

Actually, I think we kind of already have one: Tap.from_path(pn).path, so something like this should work:

diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb
index 2ad09eaa4..59cdc38b8 100644
--- a/Library/Homebrew/rubocops/extend/formula_cop.rb
+++ b/Library/Homebrew/rubocops/extend/formula_cop.rb
@@ -159,19 +159,19 @@ module RuboCop
 
       # Returns the formula tap.
       def formula_tap
-        return unless (match_obj = @file_path.match(%r{/(homebrew-\w+)/}))
+        return unless (file_tap = Tap.from_path(@file_path))
 
-        match_obj[1]
+        file_tap
       end
 
       # Returns whether the given formula exists in the given style exception list.
       # Defaults to the current formula being checked.
       def tap_style_exception?(list, formula = nil)
-        if @tap_style_exceptions.nil? && !formula_tap.nil?
+        if @tap_style_exceptions.nil? && formula_tap.present?
           @tap_style_exceptions = {}
 
-          style_exceptions_dir = "#{File.dirname(File.dirname(@file_path))}/style_exceptions/*.json"
-          Pathname.glob(style_exceptions_dir).each do |exception_file|
+          style_exceptions_dir = "style_exceptions/*.json"
+          formula_tap.path.glob(style_exceptions_dir).each do |exception_file|
             list_name = exception_file.basename.to_s.chomp(".json").to_sym
             list_contents = begin
               JSON.parse exception_file.read

The other callers of formula_tap will need fixing though. It might also be useful for maintenance to make sure that sharding logic is encoded only inside a Tap object instead of in a bunch of different places.

@MikeMcQuaid
Copy link
Member Author

+        return unless (file_tap = Tap.from_path(@file_path))

Almost certainly can't use Tap class in rubocop as it requires too much stuff which needs Homebrew's various environment variables set.

It might also be useful for maintenance to make sure that sharding logic is encoded only inside a Tap object instead of in a bunch of different places.

Generally agree but: see above 😁

@MikeMcQuaid MikeMcQuaid requested a review from a team August 9, 2023 16:15
Continuing the sharding work now that `ack.rb` was moved without issue.
@MikeMcQuaid MikeMcQuaid requested review from fxcoudert and a team August 10, 2023 13:06
Copy link
Member

@issyl0 issyl0 left a comment

Choose a reason for hiding this comment

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

🅰️

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Aug 10, 2023
Merged via the queue into Homebrew:master with commit bd23e90 Aug 10, 2023
13 checks passed
@MikeMcQuaid MikeMcQuaid deleted the shard_a branch August 10, 2023 14:29
@ZhongRuoyu ZhongRuoyu mentioned this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge-skip `brew pr-automerge` will skip this pull request CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants