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

Implement ActiveSupport's Object#blank? directly #16259

Merged
merged 8 commits into from Nov 29, 2023

Conversation

dduugg
Copy link
Sponsor Member

@dduugg dduugg commented Nov 26, 2023

  • 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%.

!**/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*
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@dduugg Fine with me as-is, thanks!

end
end

sig { returns(T::Boolean) }
def present? # :nodoc:
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

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.

@dduugg dduugg changed the title Vendor Object#blank? Implement ActiveSupport's Object#blank? directly Nov 26, 2023
@RandomDSdevel
Copy link
Contributor

RandomDSdevel commented Nov 27, 2023

(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:

  1. add 'blank?' and friends to the pre-existing 'Library/Homebrew/extend/kernel.rb'* or

  2. 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
Copy link
Sponsor Member Author

dduugg commented Nov 27, 2023

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.

@RandomDSdevel
Copy link
Contributor

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
Copy link
Member

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

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.

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.

!**/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*
Copy link
Member

Choose a reason for hiding this comment

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

@dduugg Fine with me as-is, thanks!

Library/Homebrew/extend/blank.rb Show resolved Hide resolved
@dduugg
Copy link
Sponsor Member Author

dduugg commented Nov 28, 2023

  • 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

@Bo98 Bo98 mentioned this pull request Nov 29, 2023
11 tasks
@MikeMcQuaid
Copy link
Member

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 👍🏻

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.

Great work here again @dduugg!

@MikeMcQuaid MikeMcQuaid merged commit bce8c0e into Homebrew:master Nov 29, 2023
27 checks passed
@dduugg dduugg mentioned this pull request Nov 29, 2023
7 tasks
@dduugg dduugg deleted the vendor-blank branch November 29, 2023 18:10
@github-actions github-actions bot added the outdated PR was locked due to age label Dec 30, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 30, 2023
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

5 participants