Implement ActiveSupport's Object#blank? directly #16259

dduugg · 2023-11-26T18:51:07Z

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?

As mentioned in #16190 (comment) this PR uses direct implementations of ActiveSupport's blank? monkey-patches. (The code is published under the MIT license, allowing it to be copied/modified/distributed/etc. without limitation).

The implementation does not use caching for strings with multiple encodings, as introduced in https://github.com/rails/rails/pull/31049/files, as this seems like quite the edge case to bring in concurrent-ruby for (it also shave 10ms or so off of every brew command that doesn't make use of the cache). (We could possibly use a builtin Ruby Hash for caching as a compromise, but that also seems more trouble than it is likely to be worth.)

(Note that Library/Homebrew/extend/blank.rb breaks the pattern of other files within Library/Homebrew/extend/, which are mostly concerned with patching a single module (and named accordingly), while this file introduces optimizations for several subclasses.)

By removing unused dependencies, this PR also reduces both the file and line count of the repo by > 5%.

dduugg · 2023-11-26T19:19:36Z

.gitignore

 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/object/deep_dup.rb
 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/object/duplicable.rb
 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/string/exclude.rb
 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/string/filters.rb
 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/string/indent.rb

 # Ignore partially included gems where we don't need all files
-**/vendor/bundle/ruby/*/gems/concurrent-ruby-*/lib/*/*.jar
-**/vendor/bundle/ruby/*/gems/i18n-*/lib/i18n/tests*


The i8n cleanup is unrelated, it should have been included in #15311
Let me know if you'd like a cleaner diff, and I can move this to a separate PR.

@dduugg Fine with me as-is, thanks!

dduugg · 2023-11-26T19:21:46Z

Library/Homebrew/extend/blank.rb

      end
  end
+
+  sig { returns(T::Boolean) }
+  def present? # :nodoc:


This PR uses the latest version of the code, bc we're no longer contained by version compatibility of ActiveSupport as a whole. This includes changes such as rails/rails#49909, captured here.

Library/Homebrew/extend/blank.rb

RandomDSdevel · 2023-11-27T02:28:39Z

(Note that Library/Homebrew/extend/blank.rb breaks the pattern of other files within Library/Homebrew/extend/, which are mostly concerned with patching a single module (and named accordingly), while this file introduces optimizations for several subclasses.)

It looks like, in ActiveSupport and as vendered before, their implementation is/wss under 'lib/active_support/core_ext/object/blank.rb' along with other files extending Ruby's built-in 'Object' class. Why not 'Library/Homebrew/extend/object.rb?' An RBI file containing Sorbet typing information for ActiveSupport's 'present?' method currently already exists at 'Library/Homebrew/extend/object.rbi.' (This is already an exception to the general naming pattern in 'Library/Homebrew/extend/.')
Alternatively, Ruby's'Object' class lives in its 'Kernel' module. It could therefore make sense to either:

add 'blank?' and friends to the pre-existing 'Library/Homebrew/extend/kernel.rb'* or
Move that existing file* under a new folder, 'Library/Homebrew/extend/Kernel' — in analogy to the 'ENV' and 'os' folders already in 'Library/Hombrew/extend' —, then add 'blank?' and friends into a separate file there as 'Library/Homebrew/extend/Kernel/Object/blank.rb' (modulo/mutatis mutandi:
- folder name capitalization.
- whether or not further nesting under a folder for extensions to Ruby's 'Kernel' module's 'Object' class is preferable. Options for alternative paths include:
  - 'Library/Homebrew/extend/Kernel/blank.rb' (though omitting the class extended may feel weird.)
  - 'Library/Homebrew/extend/Kernel/object.rb.')

(* There's already a to-do note in 'Library/Homebrew/extend/kernel.rb' on line 5 containing a reminder that some of the file's methods already need splitting out of that file, given how long it's gotten.)

dduugg · 2023-11-27T02:44:57Z

Why not 'Library/Homebrew/extend/object.rb?

Because there are 10 classes being monkey-patched within the file for various optimization purposes, so object.rb is IMO misleading (though, yes, technically, everything is an object). I think blank.rb is a better option, despite breaking the convention used elsewhere for extensions.

Library/Homebrew/extend/blank.rb

RandomDSdevel · 2023-11-27T05:45:40Z

Because there are 10 classes being monkey-patched within the file for various optimization purposes, so object.rb is IMO misleading (though, yes, technically, everything is an object).

Just curious: what would/might happen if you'd try to factor the 'specializations' out into their own, separate files; does that break or not work? (Probably not worth trying or worth the complexity, though; the point you made stands. Further, Ruby's duck/dynamic typing makes any even superficial thought of or attempt at exercising genrric programming kind of moot and a pointless thought experiment; there isn't any non-genrric programming. If anyone feels like iterating on code structure later, they can do that then, anyway.)

MikeMcQuaid · 2023-11-27T13:01:02Z

@RandomDSdevel Your review is not required on this PR, thanks.

MikeMcQuaid

Really happy with this so far, thanks @dduugg! The amount of lines of vendored code removed is 😍. A few questions I was unclear about:

how does blank.rb now differ from the upstream version(s)?
was concurrent-ruby removal done by us/upstream/both?
how will this be kept up-to-date? Is there any need to do so?
if we don't plan on keeping up-to-date: what do you think about stripping a bunch of the documentation/comments/any specialisations we don't really want or need?

Thanks again, suspect this will be mergeable very soon.

MikeMcQuaid · 2023-11-27T13:02:24Z

.gitignore

 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/object/deep_dup.rb
 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/object/duplicable.rb
 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/string/exclude.rb
 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/string/filters.rb
 !**/vendor/bundle/ruby/*/gems/activesupport-*/lib/active_support/core_ext/string/indent.rb

 # Ignore partially included gems where we don't need all files
-**/vendor/bundle/ruby/*/gems/concurrent-ruby-*/lib/*/*.jar
-**/vendor/bundle/ruby/*/gems/i18n-*/lib/i18n/tests*


@dduugg Fine with me as-is, thanks!

Library/Homebrew/extend/blank.rb

dduugg · 2023-11-28T18:13:53Z

how does blank.rb now differ from the upstream version(s)?

The major one is a non-concurrent cache for encodings that error: ccbb05d ccbb05d (I suspect this is better for us - Homebrew is primarily single-threaded, so this should be more performant. It also allows us to drop concurrent-ruby from the dependencies that are committed to the repo). There are also minor changes involving style fixes, and strict typing.

was concurrent-ruby removal done by us/upstream/both?

Us, as described above.

how will this be kept up-to-date? Is there any need to do so?

It will be on us to maintain. The upstream history shows a pair of very minor performance improvements in the past two months, otherwise the last code change was adding support for multiple encodings in 2017. (I can imagine writing tests to verify implementations match the upstream source, but that seems like overkill)

if we don't plan on keeping up-to-date: what do you think about stripping a bunch of the documentation/comments/any specialisations we don't really want or need?

We could do that, although we kept the documentation in the other instance that I'm aware of us incorporating ActiveSupport extensions (and we also attached the rails license as a comment): https://github.com/Homebrew/brew/blob/master/Library/Homebrew/extend/array.rb#L5-L50

MikeMcQuaid · 2023-11-29T10:49:44Z

The major one is a non-concurrent cache for encodings that error: ccbb05d ccbb05d (I suspect this is better for us - Homebrew is primarily single-threaded, so this should be more performant. It also allows us to drop concurrent-ruby from the dependencies that are committed to the repo). There are also minor changes involving style fixes, and strict typing.

All seem like great, appropriate changes 👍🏻

We could do that, although we kept the documentation in the other instance that I'm aware of us incorporating ActiveSupport extensions

Cool, fine as-is then 👍🏻

MikeMcQuaid

Great work here again @dduugg!

Per the code comment, it seems I should bump this after ignoring a pair of committed vendored gems in #16259 : https://github.com/Homebrew/brew/pull/16259/files#diff-bc37d034bad564583790a46f19d807abfe519c5671395fd494d8cce506c42947R102 https://github.com/Homebrew/brew/pull/16259/files#diff-bc37d034bad564583790a46f19d807abfe519c5671395fd494d8cce506c42947R109

dduugg added 6 commits November 26, 2023 09:33

Vendor Object#blank?

b033841

Ignore non-required dependencies

61667a8

Remove concurrent/map Object#blank? cache

ccbb05d

Strict typing

c7c539e

Style fixes

c36fafb

Remove unused active-support transitive dependencies

ff43ec2

dduugg commented Nov 26, 2023

View reviewed changes

dduugg changed the title ~~Vendor Object#blank?~~ Implement ActiveSupport's Object#blank? directly Nov 26, 2023

apainintheneck reviewed Nov 26, 2023

View reviewed changes

Library/Homebrew/extend/blank.rb Show resolved Hide resolved

Library/Homebrew/extend/blank.rb Outdated Show resolved Hide resolved

dduugg added 2 commits November 26, 2023 15:36

Use encoding cache

410fc64

Add tests

cfb8ec8

apainintheneck approved these changes Nov 27, 2023

View reviewed changes

carlocab reviewed Nov 27, 2023

View reviewed changes

Library/Homebrew/extend/blank.rb Show resolved Hide resolved

MikeMcQuaid reviewed Nov 27, 2023

View reviewed changes

Bo98 mentioned this pull request Nov 29, 2023

Ruby 3 rollout #16154

Closed

11 tasks

MikeMcQuaid approved these changes Nov 29, 2023

View reviewed changes

MikeMcQuaid merged commit bce8c0e into Homebrew:master Nov 29, 2023
27 checks passed

dduugg mentioned this pull request Nov 29, 2023

Bump VENDOR_VERSION in gems.rb #16269

Merged

7 tasks

dduugg deleted the vendor-blank branch November 29, 2023 18:10

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

github-actions bot locked as resolved and limited conversation to collaborators Dec 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ActiveSupport's Object#blank? directly #16259

Implement ActiveSupport's Object#blank? directly #16259

dduugg commented Nov 26, 2023 •

edited

Loading

dduugg Nov 26, 2023

MikeMcQuaid Nov 27, 2023

dduugg Nov 26, 2023

RandomDSdevel commented Nov 27, 2023 •

edited

Loading

dduugg commented Nov 27, 2023

RandomDSdevel commented Nov 27, 2023

MikeMcQuaid commented Nov 27, 2023

MikeMcQuaid left a comment

MikeMcQuaid Nov 27, 2023

dduugg commented Nov 28, 2023

MikeMcQuaid commented Nov 29, 2023

MikeMcQuaid left a comment

Implement ActiveSupport's Object#blank? directly #16259

Implement ActiveSupport's Object#blank? directly #16259

Conversation

dduugg commented Nov 26, 2023 • edited Loading

dduugg Nov 26, 2023

Choose a reason for hiding this comment

MikeMcQuaid Nov 27, 2023

Choose a reason for hiding this comment

dduugg Nov 26, 2023

Choose a reason for hiding this comment

RandomDSdevel commented Nov 27, 2023 • edited Loading

dduugg commented Nov 27, 2023

RandomDSdevel commented Nov 27, 2023

MikeMcQuaid commented Nov 27, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Nov 27, 2023

Choose a reason for hiding this comment

dduugg commented Nov 28, 2023

MikeMcQuaid commented Nov 29, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

dduugg commented Nov 26, 2023 •

edited

Loading

RandomDSdevel commented Nov 27, 2023 •

edited

Loading