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 style violations under newer RuboCop #16336

Merged
merged 1 commit into from Dec 14, 2023

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Dec 14, 2023

Separate PR than the actual version bump to make the diff somewhat digestable.

Library/Homebrew/cli/args.rb Outdated Show resolved Hide resolved
Comment on lines 463 to +466
def depends_on(one, two)
if one.any_installed_keg
&.runtime_dependencies
&.any? { |dependency| dependency["full_name"] == two.full_name }
&.runtime_dependencies
&.any? { |dependency| dependency["full_name"] == two.full_name }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nice.

@@ -35,8 +35,7 @@
end

it "does not use older tags when requested not to", :needs_macos do
allow(Homebrew::EnvConfig).to receive(:developer?).and_return(true)
allow(Homebrew::EnvConfig).to receive(:skip_or_later_bottles?).and_return(true)
allow(Homebrew::EnvConfig).to receive_messages(developer?: true, skip_or_later_bottles?: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also quite nice.

@Bo98 Bo98 force-pushed the style-fixes-dec2023 branch 2 times, most recently from 843eb8c to 445e146 Compare December 14, 2023 05:36
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

One very minor quibble here that can be addressed in another PR. Great job @Bo98!

@@ -25,7 +25,7 @@ def self.paths
# Return all tokens for installed casks.
sig { returns(T::Array[String]) }
def self.tokens
paths.map(&:basename).map(&:to_s)
paths.map { |path| path.basename.to_s }
Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit weird and unlike traditional Ruby patterns. Which cop suggested it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Performance/MapMethodChain. Two maps is slower than one.

Copy link
Member

Choose a reason for hiding this comment

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

@Bo98 Yeh, feels like a micro-optimisation to me and not sure I agree with it really but: not enough to object.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, it might matter over the Formula API hash where we've seen the smallest of things become visible, but probably not for any of these.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This could also be paths.map { _1.basename.to_s }, which is the most readable option imo (it avoids the duplication of map or path in the other styles).

Copy link
Member

Choose a reason for hiding this comment

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

Personally I hate the _1 variables 😅

@MikeMcQuaid MikeMcQuaid merged commit a04d8a3 into Homebrew:master Dec 14, 2023
24 checks passed
@Bo98 Bo98 deleted the style-fixes-dec2023 branch December 14, 2023 10:59
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 14, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2024
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

4 participants