Replace ActiveSupport inflections with Utils methods #14778

dduugg · 2023-02-23T05:35:32Z

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?

Based on @MikeMcQuaid's comment in #10508 (comment) I've put together a quick replacement candidate for ActiveSupport's String#pluralize monkey-patch, and ported the first five instances (based on git grep). There are ~50 instances of pluralize in the repo, so I want to confirm this approach before moving forward.

(Note that there are a handful of other String monkey-patches that I found when i remove them from the active-support rbi: five instances of demodulize, one of titleize, and one of first (which may be a type error). We will need to replace these before we can remove the active_support/core_ext/string/inflections dependency.)

(Also, after the breakages caused by #14502, I've preemptively checked homebrew-{cask,core} and found no instances of pluralize (but I didn't look for other ActiceSupport inflection methods)).

BrewTestBot · 2023-02-23T05:35:47Z

Review period will end on 2023-02-24 at 05:35:32 UTC.

MikeMcQuaid

Great work so far! Implementation looks good, just have some naming thoughts.

MikeMcQuaid · 2023-02-23T08:49:54Z

Library/Homebrew/utils/inflection.rb

+
+    # Combines `stem`` with the `singular`` or `plural` suffix based on `count`.
+    sig { params(count: Integer, stem: String, plural: String, singular: String).returns(String) }
+    def self.number(count, stem, plural = "s", singular = "")


I think pluralise (or pluralize of you prefer `en_US) may be a nicer name here.

MikeMcQuaid · 2023-02-23T08:50:23Z

Library/Homebrew/utils/inflection.rb

+  # Inflection utility methods, as a lightweight alternative to `ActiveSupport::Inflector``.
+  #
+  # @api private
+  module Inflection


I know this matches ActiveSupport but similarly perhaps making this just Utils.pluralize may be a nicer fit with the existing API.

Can do, though it feels materially different from the others at the top level:

irb(main):001:0> Utils.methods(false) => [:safe_popen_read, :git_commit_message, :popen, :git_short_head, :git_branch, :popen_read, :popen_write, :safe_popen_write, :git_head, :rewrite_child_error, :safe_fork]

@dduugg Honestly I think those ones have just ended up there because we didn't use to separate into submodules. I think the benefit of having this just be Utils::pluralize or, probably overkill, even in or wrapped in utils.rb as pluralize is it makes invocations, usually interpolated, much shorter.

Rylan12 · 2023-02-23T16:08:44Z

Library/Homebrew/utils/inflection.rb

+
+    # Combines `stem`` with the `singular`` or `plural` suffix based on `count`.
+    sig { params(count: Integer, stem: String, plural: String, singular: String).returns(String) }
+    def self.number(count, stem, plural = "s", singular = "")


Thoughts on making plural and singular keyword rags? Could even abbreviate to pl and sg if line length is a concern. I think it will be much easier to use the methods this way since remembering the order won't be necessary.

Suggested change

def self.number(count, stem, plural = "s", singular = "")

def self.number(count, stem, plural: "s", singular: "")

I also like the pluralize naming, and if we go with that I think it makes sense to have the stem come first. So ultimately:

Suggested change

def self.number(count, stem, plural = "s", singular = "")

def self.pluralize(stem, count, plural: "s", singular: "")

dduugg · 2023-02-23T16:38:38Z

I'll go with the majority and call it pluralize, but will explain my original thinking here. Grammatical number is the concept we're implementing, so i chose number for that reason. (pluralize makes sense when it's acting on the singular noun, but this is starting in an undermined state).

I don't have a strong opinion about parameter ordering or kwarg vs positional, so i'll go with @Rylan12's recommendation there.

dduugg · 2023-02-23T23:32:19Z

I've changed the name to pluralize, used the method signature suggested by @Rylan12, ported a few more call sites, and added a non-monkey-patched demodulize. These methods remain under Utils::Inflection for now. There's some odd formatting due to brew style --fix that i'll clean up ahead of final review.

PTAL @Rylan12 @MikeMcQuaid

BrewTestBot · 2023-02-24T06:02:07Z

Review period ended.

…ils|g'

dduugg · 2023-02-28T05:11:43Z

@MikeMcQuaid @Rylan12 PTAL

Changed the namespace from Utils::Inflections to Utils
Also ported demodulize and deconstantize from ActiveSupport::Inflections, and inlined an instance of titleize
Added tests

require "active_support/core_ext/string/inflections" remains in global.rb. I'd like to scope that to a separate PR in case we see downstream consequences (like we did in #14502 ) and wish to revert.

dduugg · 2023-02-28T05:25:08Z

Library/Homebrew/sorbet/rbi/gems/activesupport@6.1.7.2.rbi

@@ -3415,56 +3415,6 @@ class Regexp::Token < ::Struct
  end
 end

-class String


This serves as a type guard to avoid introducing monkey-patch dependencies into typed code while the deletion of require "active_support/core_ext/string/inflections" in global.rb is pending

dduugg · 2023-02-28T05:27:20Z

Library/Homebrew/utils/repology.rb

@@ -76,7 +76,8 @@ def parse_api_response(limit = nil, last_package = "", repository:)
      break if (limit && outdated_packages.size >= limit) || response.size <= 1
    end

-    puts "#{outdated_packages.size} outdated #{package_term.pluralize(outdated_packages.size)} found"
+    package_term = package_term.chop if outdated_packages.size == 1


This was the only dynamic instance of pluralize. I took advantage of the fact that there's an easier path from plural to singular among {formulae, casks, packages} and reversed the logic here

dduugg · 2023-02-28T05:29:13Z

Library/Homebrew/livecheck/strategy/git.rb

@@ -103,7 +103,8 @@ def self.versions_from_tags(tags, regex = nil, &block)
          tags.map do |tag|
            if regex
              # Use the first capture group (the version)
-              tag.scan(regex).first&.first
+              # This code is not typesafe unless the regex includes a capture group
+              T.unsafe(tag.scan(regex).first)&.first


This is a type error that appears when deleting String#first from the active support rbi below (but I don't think this code uses that path, looking at call sites).

dduugg · 2023-02-28T05:30:30Z

Library/Homebrew/cask/cmd/uninstall.rb

@@ -46,7 +46,7 @@ def self.uninstall_casks(*casks, binaries: nil, force: false, verbose: false)
          next if (versions = cask.versions).empty?

          puts <<~EOS
-            #{cask} #{versions.to_sentence} #{"is".pluralize(versions.count)} still installed.
+            #{cask} #{versions.to_sentence} #{versions.count == 1 ? "is" : "are"} still installed.


it's possible that versions.one? makes sense in substitutions like these, but there are some edge cases i didn't want to risk

MikeMcQuaid

Looks good, nice cleanup!

MikeMcQuaid · 2023-02-28T11:20:50Z

Thanks again @dduugg!

This was replaced by a method in `Utils`, but we don't really need that here either. See Homebrew/brew#14778.

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 23, 2023

dduugg requested a review from MikeMcQuaid February 23, 2023 05:35

dduugg force-pushed the inflection-util branch from b4ee45d to 1e16019 Compare February 23, 2023 05:37

dduugg marked this pull request as draft February 23, 2023 05:56

MikeMcQuaid reviewed Feb 23, 2023

View reviewed changes

Rylan12 reviewed Feb 23, 2023

View reviewed changes

dduugg force-pushed the inflection-util branch from 7a937a7 to 2394572 Compare February 24, 2023 00:11

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 24, 2023

BrewTestBot approved these changes Feb 24, 2023

View reviewed changes

dduugg force-pushed the inflection-util branch 5 times, most recently from a8b4a6f to 3420278 Compare February 24, 2023 17:53

dduugg marked this pull request as ready for review February 24, 2023 18:11

dduugg marked this pull request as draft February 24, 2023 18:18

dduugg mentioned this pull request Feb 24, 2023

Move Kernel extensions into a separate file #14805

Merged

7 tasks

dduugg added 9 commits February 27, 2023 20:17

Draft implementation to replace ActiveSupport inflections

0438a3a

Change to pluralize, port more call sites

37015b6

Replace demodulize inflection with util

d62211d

brew style --fix

2400c6d

Replace deconstantize inflection with util

2c73d4d

Port more call sites

eb2b990

Port more call sites

3da6865

Port dynamic invocation

2c5067e

inline titleize

7f3593e

dduugg added 8 commits February 27, 2023 20:18

Fix type error

cfdb0c2

Try removing String monkey-patched types

8d788ec

Try removing inflections require

86c500f

git grep -l Utils::Inflection | xargs gsed -i 's|Utils::Inflection|Ut…

0eccc0e

…ils|g'

Move inflection utils to Utils module

325994a

Add tests

6ea501b

Restore global

82dad85

Fix style/type violations

1ab278f

dduugg force-pushed the inflection-util branch 2 times, most recently from ec43833 to 4ab2f20 Compare February 28, 2023 04:52

dduugg marked this pull request as ready for review February 28, 2023 05:11

dduugg force-pushed the inflection-util branch from 4ab2f20 to 7f042c6 Compare February 28, 2023 05:19

dduugg commented Feb 28, 2023

View reviewed changes

dduugg added 2 commits February 27, 2023 21:42

Tidy up

f9f73f3

brew style --fix

2073e70

dduugg force-pushed the inflection-util branch from 743e28d to 2073e70 Compare February 28, 2023 05:43

dduugg changed the title ~~Draft implementation to replace ActiveSupport inflections~~ Replace ActiveSupport inflections with Utils methods Feb 28, 2023

MikeMcQuaid approved these changes Feb 28, 2023

View reviewed changes

MikeMcQuaid merged commit 5ca4ab0 into Homebrew:master Feb 28, 2023

dduugg mentioned this pull request Mar 1, 2023

Move Array#to_sentence monkey-patch from ActiveSupport to extend/ #14847

Merged

7 tasks

apainintheneck mentioned this pull request Mar 1, 2023

cask/cmd/upgrade: fix missing versions variable #14848

Merged

7 tasks

carlocab mentioned this pull request Mar 8, 2023

Remove use of pluralize Homebrew/homebrew-test-bot#882

Merged

carlocab added a commit to carlocab/homebrew-test-bot that referenced this pull request Mar 8, 2023

Remove use of pluralize

2c6ea0d

This was replaced by a method in `Utils`, but we don't really need that here either. See Homebrew/brew#14778.

github-actions bot added the outdated PR was locked due to age label Apr 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Apr 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ActiveSupport inflections with Utils methods #14778

Replace ActiveSupport inflections with Utils methods #14778

dduugg commented Feb 23, 2023 •

edited

BrewTestBot commented Feb 23, 2023

MikeMcQuaid left a comment

MikeMcQuaid Feb 23, 2023

MikeMcQuaid Feb 23, 2023

dduugg Feb 23, 2023

MikeMcQuaid Feb 24, 2023

Rylan12 Feb 23, 2023

dduugg commented Feb 23, 2023

dduugg commented Feb 23, 2023

BrewTestBot commented Feb 24, 2023

dduugg commented Feb 28, 2023

dduugg Feb 28, 2023

dduugg Feb 28, 2023

dduugg Feb 28, 2023

dduugg Feb 28, 2023

MikeMcQuaid left a comment

MikeMcQuaid commented Feb 28, 2023

	def self.number(count, stem, plural = "s", singular = "")
	def self.number(count, stem, plural: "s", singular: "")

	def self.number(count, stem, plural = "s", singular = "")
	def self.pluralize(stem, count, plural: "s", singular: "")

Replace ActiveSupport inflections with Utils methods #14778

Replace ActiveSupport inflections with Utils methods #14778

Conversation

dduugg commented Feb 23, 2023 • edited

BrewTestBot commented Feb 23, 2023

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

dduugg commented Feb 23, 2023

dduugg commented Feb 23, 2023

BrewTestBot commented Feb 24, 2023

dduugg commented Feb 28, 2023

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

MikeMcQuaid commented Feb 28, 2023

dduugg commented Feb 23, 2023 •

edited