Vendor remaining Rails cops, remove ActiveSupport #16510

dduugg · 2024-01-19T22:14:42Z

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 typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Resolves #16190

Begins replacing Rails cops as advised in #16190 (comment)
Three cops in particular can be replaced with Style/InverseMethods and a small subset of their prior logic, which I've attempted in this PR.

dduugg · 2024-01-19T23:26:50Z

Library/Homebrew/sorbet/rbi/hidden-definitions/hidden.rbi

@@ -9357,10 +9360,18 @@ module RuboCop::Cop::HelperFunctions
  extend ::T::Private::Methods::SingletonMethodHooks
 end

+class RuboCop::Cop::Homebrew::Blank
+  def nil_or_empty?(param0=T.unsafe(nil)); end


An update was necessary to pick up these def_node_matcher methods.

apainintheneck · 2024-01-20T05:36:02Z

Library/Homebrew/test/rubocops/blank_spec.rb

+describe RuboCop::Cop::Homebrew::Blank do
+  subject(:cop) { described_class.new }
+
+  shared_examples "offense" do |source, correction, message|


Stupid question: would it be possible to use kwargs here to improve readability?

Possibly! Though I should have added that the test cases are copy/pasta from upstream, where the test cases are appropriate, and I'm not especially inclined to drift from it:

https://github.com/rubocop/rubocop-rails/blob/master/spec/rubocop/cop/rails/blank_spec.rb#L4

(Source code made available under the MIT license, which may be appropriate to paste into the new files here, depending on one's view of "substantial portions".)

apainintheneck

LGTM

Library/Homebrew/startup/bootsnap.rb

Bo98 · 2024-01-20T05:58:54Z

Library/.rubocop.yml

+Style/InverseMethods:
+  Exclude:
+    # Core extensions are not available here:
+    - "Homebrew/standalone/init.rb"
+    - "Homebrew/rubocops/**/*"


I'm guessing this also disables vanilla Ruby InverseMethods checking.

If so, maybe worth having a InverseMethodsExtension cop that is nothing but a subclass of RuboCop::Cop::Style::InverseMethods, but will I think allow us to separate configs? You can ignore this suggestion if you prefer though and I can maybe look into that more later.

Thanks for the call out, I should have added a bit more on my thought process. Let's enumerate some options here, because there are several. More or less in my order of preference, they include:

Add :exclude?: :include? here, but exclude Style/InverseMethods cop where exclude? is unavailable (what I've done above)

Omit :exclude?: :include? from the cop, as we do with Style/InvertibleUnlessCondition below

Add :exclude?: :include? to the cop, use inline disable comments where exclude? is unavailable

Just import extend/string and/or extend/array anywhere there's a violation, once some version of Remove ActiveSupport from runtime #16463 lands

Omit :exclude?: :include? from the cop, vendor the Rails/NegateInclude include cop (perhaps a simpler, though less extensible, version of what you're suggesting, IIUC)

Add an InverseMethodsExtension cop that is nothing but a subclass of RuboCop::Cop::Style::InverseMethods, but would allow us to separate configs

Remove the ActiveSupport dependency in rubocop-rails (a long shot, but hey)

I'm generally biased toward whichever results in the least amount of add'l code/comments, and I'm pretty ok if we don't have 100% coverage of InverseMethods. That said, I'm happy to do any of these (except the last one 🙈) if there is consensus otherwise. Let me know if this is helpful/influences where you think we should go with this.

Omit :exclude?: :include? from the cop, vendor the Rails/NegateInclude include cop (perhaps a simpler, though less extensible, version of what you're suggesting, IIUC)

Add an InverseMethodsExtension cop that is nothing but a subclass of RuboCop::Cop::Style::InverseMethods, but would allow us to separate configs

either of these sound nicest to me!

dduugg · 2024-01-20T18:14:53Z

Library/Homebrew/extend/blank.rb

-  def blank?
-    true
-  end
+  def blank? = true


A bit of Ruby 3 cleanup while were dealing with blank?. (Since these work like constants, I quite prefer this style, but lmk if this inappropriate for the scope of this PR.)

This looks good to me!

MikeMcQuaid

Looks good so far! I'd be fine with merging as-is and iterating on comments, if desired. Thanks @dduugg!

MikeMcQuaid · 2024-01-22T12:42:23Z

Library/.rubocop.yml

+Homebrew/Blank:
+  Exclude:
+    # Core extensions are not available here:
+    - "Homebrew/rubocops/**/*"
+    - "Homebrew/startup/bootsnap.rb"


Don't want to blow up the scope but: I almost wonder whether it might be nicer still to have Homebrew/Blank specifically check that blank? is not used in these files?

Hmm, my gut instinct is that is a weird scope for a single cop (use a method one way in these paths, and not at all in these others). Possibly we could take inspiration from Lint/DeprecatedClassMethods, use a version of it to ban blank?, and scope the cop to these files.

I do think its worth taking a step back and thinking about whether we should just require extend/blank in these paths though. It seems like less trouble than maintaining separate guard rails based on paths, especially given that workspaces are generally poorly contemplated in Ruby and associated tooling (e.g. sorbet) generally.

It turns out we already rely on extend/blank in our rubocops, because it was transitively available via rubocop-rails: https://github.com/Homebrew/brew/blob/8e9d294/Library/Homebrew/rubocops/shell_commands.rb#L123

Mind if I explicitly require it in our rubocops for now? We can circle back and refactor it away later, but i think its reasonable to maintain the status quo in this PR.

MikeMcQuaid · 2024-01-22T12:43:33Z

Library/.rubocop.yml

+Style/InverseMethods:
+  Exclude:
+    # Core extensions are not available here:
+    - "Homebrew/standalone/init.rb"
+    - "Homebrew/rubocops/**/*"


Omit :exclude?: :include? from the cop, vendor the Rails/NegateInclude include cop (perhaps a simpler, though less extensible, version of what you're suggesting, IIUC)

Add an InverseMethodsExtension cop that is nothing but a subclass of RuboCop::Cop::Style::InverseMethods, but would allow us to separate configs

either of these sound nicest to me!

MikeMcQuaid · 2024-01-22T12:44:22Z

Library/Homebrew/extend/blank.rb

-  def blank?
-    true
-  end
+  def blank? = true


This looks good to me!

dduugg · 2024-01-26T22:51:12Z

Library/.rubocop.yml

@@ -355,15 +344,18 @@ Style/HashAsLastArrayItem:
    - "/**/Formula/**/*.rb"
    - "**/Formula/**/*.rb"

+Style/InverseMethods:
+  InverseMethods:
+    :blank?: :present?


This, and the InvertibleUnlessCondition entry below it, allow us to remove some redundant contents of the Blank and Present cops (specificallye, the NotPresent, UnlessPresent, NotBlank, and UnlessBlank config option handling.

dduugg · 2024-01-26T22:51:52Z

Library/.rubocop.yml

 Style/InvertibleUnlessCondition:
  Enabled: true
  InverseMethods:
    # Favor `if a != b` over `unless a == b`
    :==: :!=
    # Unset this (prefer `unless a.zero?` over `if a.nonzero?`)
    :zero?:
-    # Don't require non-standard `exclude?` (for now at least) - it's not available in every file
-    :include?:


This was introduced by the rubocop-rails config, and is no longer necessary to unset.

dduugg · 2024-01-26T22:53:45Z

Library/Homebrew/cask/artifact/abstract_artifact.rb

@@ -155,7 +155,7 @@ def to_s
      end

      def to_args
-        @dsl_args.reject(&:blank?)
+        @dsl_args.compact_blank


I'm a bit confused why these violations were not caught before 😕

dduugg · 2024-01-26T22:54:56Z

Library/Homebrew/rubocops/dependency_order.rb

@@ -75,7 +75,7 @@ def sort_conditional_dependencies!(ordered)
            ordered.each_with_index do |dep, pos|
              idx = pos+1
              match_nodes = build_with_dependency_name(dep)
-              next if !match_nodes || match_nodes.empty?
+              next if match_nodes.blank?


As noted above, blank? is been available (and used) within our custom rubocops already, so i've removed the exclusions, pending refactor

dduugg · 2024-01-26T22:55:50Z

Library/Homebrew/test/global_spec.rb

-
-  it "does not require i18n" do
-    # This is a transitive dependency of activesupport, but we don't use it.
-    expect { I18n }.to raise_error(NameError)


This no longer passes typechecking, bc the i18n rbi file is gone. I don't think it's an especially useful test anymore, so let's just yank it.

dduugg · 2024-01-26T23:00:29Z

I've updated the PR to vendor the remaining Rails cops and thus remove rupocop-rails and activesupport.

This makes for a large PR, and it would be reasonable to request to break them into 2 PRs instead, just let me know.

As currently scoped, this PR removes > 8% of the checked-in code (by number of lines). 📉

MikeMcQuaid · 2024-01-28T16:07:47Z

Great work, thanks again @dduugg!

dduugg marked this pull request as draft January 19, 2024 22:14

dduugg force-pushed the inverse-include-exclude-cop branch 4 times, most recently from 2850c5e to b963260 Compare January 19, 2024 23:16

dduugg changed the title ~~Replace Rails/NegateInclude with Inverse/Invertible cops~~ Replace some Rails cops with Style/InverseMethods and custom versions Jan 19, 2024

dduugg commented Jan 19, 2024

View reviewed changes

dduugg force-pushed the inverse-include-exclude-cop branch from b72a730 to fe0fbb1 Compare January 19, 2024 23:32

dduugg marked this pull request as ready for review January 19, 2024 23:37

apainintheneck reviewed Jan 20, 2024

View reviewed changes

apainintheneck approved these changes Jan 20, 2024

View reviewed changes

Bo98 approved these changes Jan 20, 2024

View reviewed changes

Library/Homebrew/startup/bootsnap.rb Outdated Show resolved Hide resolved

Bo98 reviewed Jan 20, 2024

View reviewed changes

dduugg commented Jan 20, 2024

View reviewed changes

MikeMcQuaid approved these changes Jan 22, 2024

View reviewed changes

Replace Rails/NegateInclude with Inverse/Invertible cops

9d081a6

dduugg force-pushed the inverse-include-exclude-cop branch 2 times, most recently from d42c0e6 to b945574 Compare January 26, 2024 22:15

dduugg changed the title ~~Replace some Rails cops with Style/InverseMethods and custom versions~~ Vendor remaining Rails cops, remove ActiveSupport Jan 26, 2024

dduugg commented Jan 26, 2024

View reviewed changes

dduugg force-pushed the inverse-include-exclude-cop branch from e0ca750 to 9c8219a Compare January 26, 2024 23:02

dduugg added 8 commits January 26, 2024 15:03

Remove inline disables

686264f

Prefer endless methods

6472aca

Vendor CompactBlank cop

f99d39f

Vendor Presence cop

665bda0

Vendor SafeNavigationWithBlank cop

0caaa1f

Vendor InverseMethods cop

ae249ec

brew vendor-gems: commit updates.

2e90749

Remove .rbi files for removed dependencies

8f52b6f

dduugg added 2 commits January 26, 2024 15:04

brew typecheck --update-all

8db86f7

Resolve type errors

4b59101

dduugg force-pushed the inverse-include-exclude-cop branch from 9c8219a to 4b59101 Compare January 26, 2024 23:04

Remove stray ActiveSupport references

bec27d4

dduugg force-pushed the inverse-include-exclude-cop branch from 47e66b6 to bec27d4 Compare January 26, 2024 23:38

apainintheneck approved these changes Jan 27, 2024

View reviewed changes

dduugg merged commit 3a27cac into Homebrew:master Jan 30, 2024
26 checks passed

dduugg deleted the inverse-include-exclude-cop branch January 30, 2024 19:59

github-actions bot added the outdated PR was locked due to age label Mar 1, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor remaining Rails cops, remove ActiveSupport #16510

Vendor remaining Rails cops, remove ActiveSupport #16510

dduugg commented Jan 19, 2024 •

edited

Loading

dduugg Jan 19, 2024

apainintheneck Jan 20, 2024

dduugg Jan 20, 2024

apainintheneck left a comment

Bo98 Jan 20, 2024 •

edited

Loading

dduugg Jan 20, 2024

MikeMcQuaid Jan 22, 2024

dduugg Jan 20, 2024

MikeMcQuaid Jan 22, 2024

MikeMcQuaid left a comment

MikeMcQuaid Jan 22, 2024

dduugg Jan 26, 2024

dduugg Jan 26, 2024

MikeMcQuaid Jan 22, 2024

MikeMcQuaid Jan 22, 2024

dduugg Jan 26, 2024

dduugg Jan 26, 2024

dduugg Jan 26, 2024

dduugg Jan 26, 2024

dduugg Jan 26, 2024

dduugg commented Jan 26, 2024

MikeMcQuaid commented Jan 28, 2024

Vendor remaining Rails cops, remove ActiveSupport #16510

Vendor remaining Rails cops, remove ActiveSupport #16510

Conversation

dduugg commented Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apainintheneck left a comment

Choose a reason for hiding this comment

Bo98 Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Jan 26, 2024

MikeMcQuaid commented Jan 28, 2024

dduugg commented Jan 19, 2024 •

edited

Loading

Bo98 Jan 20, 2024 •

edited

Loading