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

Additional Ruby 3 fixes relating to kwarg changes #80

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

jamesbrooks
Copy link
Contributor

Fixes some failures when using Ruby 3 in situations where it appears a few layers of delegation occur inside other libraries (in this case I was having failures using batch-loader in conjunction with view_component or dry-initializer).

This change passes the current tests on both ruby 3 and previous ruby versions. Sorry for not being able to include a test directly to replicate this issue I haven't dug into exactly what those other gems are doing to trigger this issue but the changes here should be innocent enough to not cause too much alarm I hope! :)

@jeffmeyers
Copy link

Hi, any movement on merging this into a release? Anything I can do to help? We see the same deprecation warnings and are preparing to upgrade to Ruby 3.

@tony-pizza
Copy link

Bump! batch-loader is not safe to use in Ruby 3 without these changes!

Here's a patch to add a spec if that's the blocker here. It fails in Ruby 3 on master and passes in this branch.

Thanks @jamesbrooks!

diff --git i/spec/batch_loader_spec.rb w/spec/batch_loader_spec.rb
index 7755c96..ab36b6b 100644
--- i/spec/batch_loader_spec.rb
+++ w/spec/batch_loader_spec.rb
@@ -85,6 +85,19 @@ RSpec.describe BatchLoader do

       expect(result).to eq([1, 2])
     end
+
+    it 'forwards arguments' do
+      user = User.save(id: 1)
+      post = Post.new(user_id: user.id)
+
+      batch_loader = post.user_lazy
+
+      expect(
+        batch_loader.method_with_arg_kwarg_and_block(1, b: 2) do |a, b|
+          a + b
+        end
+      ).to eq(3)
+    end
   end

   context 'with custom key' do
diff --git i/spec/fixtures/models.rb w/spec/fixtures/models.rb
index 6a45064..5391c8f 100644
--- i/spec/fixtures/models.rb
+++ w/spec/fixtures/models.rb
@@ -81,6 +81,11 @@ class User
     other.is_a?(User) && id == other.id
   end

+  # Used to test argument forwarding
+  def method_with_arg_kwarg_and_block(a, b:, &block)
+    block.call(a, b)
+  end
+
   private

   def some_private_method

@franee
Copy link

franee commented Nov 8, 2023

Bump! @exAspArk can you merge this please? I confirm this fixes my issue with ruby 3.0.6 and rails 6.1.7.6.

@exAspArk exAspArk merged commit 24c9ee2 into exAspArk:master Nov 23, 2023
@exAspArk
Copy link
Owner

@jamesbrooks @tony-pizza thank you for submitting this PR and sharing your code!
@jeffmeyers @franee thank you for bumping this and sorry for the huge delay!

💛

I just released these changes in version 2.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants