Fix Delegator to pass keyword arguments for Ruby 3.0 #1684

andrewtblake · 2021-03-19T17:16:46Z

When Ruby is upgraded to 3.0, errors result from keyword arguments not being passed correctly through the Delegator delegate method. This PR fixes the issue.

dentarg · 2021-03-20T00:48:05Z

Looks like this change isn't compatible with Ruby < 2.7

Would also be great to have a test for this (which fails on Ruby 3.0 until the problem has been fixed)

andrewtblake · 2021-03-20T12:59:30Z

Apologies for being a PR newbie. I will work on providing a test, but maybe an informal explanation will help in the meantime: In a before.rb controller file, a call like before accepted_verbs: [ 'PUT', 'POST' ] do ... fails in Ruby 3, because the before method in sinatra base.rb L1387 receives the hash { accepted_verbs: ['PUT'', 'POST' ] } in its path parameter, instead of as part of **options where it is expecting it -- which causes the Mustermann.new method later on to complain that it doesn't know what to do with a Hash I believe the origin of the issue is Delegator.delegate not passing parameters through in a way that Ruby 3 can deal with, hence the changes I suggested in the PR I don't really have any idea how to make my proposed changes work also in Ruby < 2.7

…

On Sat, 20 Mar 2021 at 00:48, Patrik Ragnarsson ***@***.***> wrote: Looks like this change isn't compatible with Ruby < 2.7 Would also be great to have a test for this (which fails on Ruby 3.0 until the problem has been fixed) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1684 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACHILEBB3Z4TR3Y33Q2NGULTEPWFJANCNFSM4ZPF5J3Q> .

andrewtblake · 2021-03-20T15:25:02Z

I'm not sure of the best way to provide you with my suggested test, but if you want to try it manually it would be something like the following added to test/delegator_test.rb starting at line 91: ==================== it "delegates before with keyword arguments correctly" do delegation_app do set(:foo) do |something| something end before(foo: 'bar') do :nothing end end end ================== With the original base.rb (before my changes) this should - pass on Ruby < 2.7 - pass with a warning on Ruby >= 2.7, < 3.0 - fail on Ruby >= 3.0, producing TypeError: Hash can't be coerced into Mustermann::Pattern After my changes, the test should - pass on Ruby >= 2.7 - among 72 other failures, give a weird ArgumentError on the new line 93 of test/delegator_test.rb, when set(:foo) is called -- on Ruby < 2.7 Hope this helps On Sat, 20 Mar 2021 at 12:59, Andrew Thomas Blake < ***@***.***> wrote:

…

Apologies for being a PR newbie. I will work on providing a test, but maybe an informal explanation will help in the meantime: In a before.rb controller file, a call like before accepted_verbs: [ 'PUT', 'POST' ] do ... fails in Ruby 3, because the before method in sinatra base.rb L1387 receives the hash { accepted_verbs: ['PUT'', 'POST' ] } in its path parameter, instead of as part of **options where it is expecting it -- which causes the Mustermann.new method later on to complain that it doesn't know what to do with a Hash I believe the origin of the issue is Delegator.delegate not passing parameters through in a way that Ruby 3 can deal with, hence the changes I suggested in the PR I don't really have any idea how to make my proposed changes work also in Ruby < 2.7 On Sat, 20 Mar 2021 at 00:48, Patrik Ragnarsson ***@***.***> wrote: > Looks like this change isn't compatible with Ruby < 2.7 > > Would also be great to have a test for this (which fails on Ruby 3.0 > until the problem has been fixed) > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#1684 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACHILEBB3Z4TR3Y33Q2NGULTEPWFJANCNFSM4ZPF5J3Q> > . >

- passes on Ruby < 2.7 - passes with warning on Ruby >= 2.7, < 3.0 - fails on Ruby 3.0

dentarg · 2021-03-22T11:52:50Z

@andrewtblake You can add your test from #1686 to this branch

From reading https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html it sounds like we need to use ruby2_keywords. Maybe you can experiment with it?

At https://github.com/ruby/ruby2_keywords#usage there's an example that is using define_method

andrewtblake · 2021-03-22T20:42:54Z

That looks like it will do the trick, thanks. I will work on putting together a new PR including the test and the ruby2_keywords approach.

…

On Mon, 22 Mar 2021 at 11:53, Patrik Ragnarsson ***@***.***> wrote: @andrewtblake <https://github.com/andrewtblake> You can add your test from #1686 <#1686> to this branch From reading https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html it sounds like we need to use ruby2_keywords <https://github.com/ruby/ruby2_keywords>. Maybe you can experiment with it? At https://github.com/ruby/ruby2_keywords#usage there's an example that is using define_method — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1684 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACHILECBA3H7IZ6YS2QV7X3TE4VSHANCNFSM4ZPF5J3Q> .

andrewtblake · 2021-03-22T21:11:51Z

If you have time, I'd appreciate some guidance on where is the best place to put the gem "ruby2_keywords", "~> 0.0.4" line in Gemfile

…

-- should it be inside the if RUBY_ENGINE == "ruby" block? Or above, where gem "twitter-text", "1.14.7" is? Also, do you prefer the require 'ruby2_keywords' line to be right above where it is used, or alternatively at the top of the base.rb file with the other requires? Thanks for your help On Mon, 22 Mar 2021 at 20:42, Andrew Thomas Blake < ***@***.***> wrote:

That looks like it will do the trick, thanks. I will work on putting together a new PR including the test and the ruby2_keywords approach. On Mon, 22 Mar 2021 at 11:53, Patrik Ragnarsson ***@***.***> wrote: > @andrewtblake <https://github.com/andrewtblake> You can add your test > from #1686 <#1686> to this branch > > From reading > https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html > it sounds like we need to use ruby2_keywords > <https://github.com/ruby/ruby2_keywords>. Maybe you can experiment with > it? > > At https://github.com/ruby/ruby2_keywords#usage there's an example that > is using define_method > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1684 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACHILECBA3H7IZ6YS2QV7X3TE4VSHANCNFSM4ZPF5J3Q> > . >

dentarg

Nice work, I answered your questions about Gemfile/require

dentarg · 2021-03-23T08:57:50Z

lib/sinatra/base.rb

+
+  # We need ruby2_keywords to make delegation work correctly on
+  # ruby2.7 and ruby3
+  require 'ruby2_keywords'


I would put the require at the top of the file, somewhere at

sinatra/lib/sinatra/base.rb

Lines 4 to 10 in cf40452

# external dependencies

require 'rack'

require 'tilt'

require 'rack/protection'

require 'mustermann'

require 'mustermann/sinatra'

require 'mustermann/regular'

dentarg · 2021-03-23T08:59:16Z

Gemfile

@@ -66,6 +66,7 @@ if RUBY_ENGINE == "ruby"
  gem 'commonmarker', '~> 0.20.0'
  gem 'pandoc-ruby', '~> 2.0.2'
  gem 'simplecov', require: false
+  gem 'ruby2_keywords', '~> 0.0.4'


The Gemfile only holds our test dependencies, this will be a runtime dependency for Sinatra, so it needs to be in the gemspec:

sinatra/sinatra.gemspec

Lines 46 to 49 in cf40452

s.add_dependency 'rack', '~> 2.2'

s.add_dependency 'tilt', '~> 2.0'

s.add_dependency 'rack-protection', version

s.add_dependency 'mustermann', '~> 1.0'

dentarg · 2021-03-23T09:29:00Z

lib/sinatra/base.rb

@@ -1947,6 +1952,7 @@ def self.delegate(*methods)
          return super(*args, &block) if respond_to? method_name
          Delegator.target.send(method_name, *args, &block)
        end
+        ruby2_keywords method_name


We can also avoid needing to use the ruby2_keywords gem if we do

Suggested change

ruby2_keywords method_name

ruby2_keywords(method_name) if respond_to?(:ruby2_keywords, true)

andrewtblake · 2021-03-23T09:58:46Z

That seems much cleaner, although apparently the gem is already a dependency of mustermann 1.1.1

…

On Tue, 23 Mar 2021 at 09:29, Patrik Ragnarsson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/sinatra/base.rb <#1684 (comment)>: > @@ -1947,6 +1952,7 @@ def self.delegate(*methods) return super(*args, &block) if respond_to? method_name Delegator.target.send(method_name, *args, &block) end + ruby2_keywords method_name We can also avoid needing to use the ruby2_keywords gem if we do ⬇️ Suggested change - ruby2_keywords method_name + ruby2_keywords(method_name) if respond_to?(:ruby2_keywords, true) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1684 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACHILEH4SCRWZDNLGMHKTFDTFBNPRANCNFSM4ZPF5J3Q> .

dentarg · 2021-03-25T10:28:53Z

lib/sinatra/base.rb

@@ -1924,6 +1924,7 @@ class #{self.class}
  # inherit all settings, routes, handlers, and error pages from the
  # top-level. Subclassing Sinatra::Base is highly recommended for
  # modular applications.
+


Let's drop this newline

dentarg · 2021-03-25T11:56:00Z

@andrewtblake if you want you can squash all commits here so it's just one so we get a nicer history

FYI I'm not a maintainer so I can't merge this

Relocate ruby2_keywords gem dependency and `require` line Condition use of ruby2_keywords to avoid introducing gem dependency Drop superfluous newline

jkowens · 2021-03-25T14:36:19Z

Looks good to me! Thanks @andrewtblake and @dentarg 👍

Use sinatra PR for ruby-3 compatibility sinatra/sinatra#1684 Remove sinatra Bump bundler version Bump ruby version Temporarily disable rubocop/reek to focus on ruby-3

Fix Delegator to pass keyword arguments for Ruby 3.0

Fix Delegator to pass keyword arguments for Ruby 3.0

1d854b7

Add test for delegation of keyword arguments in before method

24fd287

- passes on Ruby < 2.7 - passes with warning on Ruby >= 2.7, < 3.0 - fails on Ruby 3.0

Merge remote-tracking branch 'origin/add_test_on_delegator_before'

15d8aca

dentarg reviewed Mar 23, 2021

View reviewed changes

dentarg reviewed Mar 25, 2021

View reviewed changes

dentarg approved these changes Mar 25, 2021

View reviewed changes

Use ruby2_keywords to pass keyword arguments correctly in delegation

e767648

Relocate ruby2_keywords gem dependency and `require` line Condition use of ruby2_keywords to avoid introducing gem dependency Drop superfluous newline

andrewtblake force-pushed the master branch from 60d4a24 to e767648 Compare March 25, 2021 12:43

jkowens requested a review from namusyaka March 25, 2021 14:36

epergo approved these changes Apr 3, 2021

View reviewed changes

alex-damian-negru approved these changes Apr 6, 2021

View reviewed changes

namusyaka approved these changes Apr 17, 2021

View reviewed changes

namusyaka merged commit ac5404d into sinatra:master Apr 17, 2021

namusyaka added a commit to namusyaka/sinatra that referenced this pull request Apr 17, 2021

Merge pull request sinatra#1684 from andrewtblake/master

e434219

Fix Delegator to pass keyword arguments for Ruby 3.0

namusyaka added a commit that referenced this pull request Apr 17, 2021

Merge pull request #1684 from andrewtblake/master

22880f2

Fix Delegator to pass keyword arguments for Ruby 3.0

dentarg mentioned this pull request Sep 22, 2021

Sinatra v3.0.0 #1531

Closed

dentarg mentioned this pull request Jan 3, 2022

Update changelog since last release #1733

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Delegator to pass keyword arguments for Ruby 3.0 #1684

Fix Delegator to pass keyword arguments for Ruby 3.0 #1684

andrewtblake commented Mar 19, 2021

dentarg commented Mar 20, 2021

andrewtblake commented Mar 20, 2021 via email

andrewtblake commented Mar 20, 2021 via email

dentarg commented Mar 22, 2021

andrewtblake commented Mar 22, 2021 via email

andrewtblake commented Mar 22, 2021 via email

dentarg left a comment

dentarg Mar 23, 2021

dentarg Mar 23, 2021

dentarg Mar 23, 2021

andrewtblake commented Mar 23, 2021 via email

dentarg Mar 25, 2021

dentarg commented Mar 25, 2021

jkowens commented Mar 25, 2021

	# external dependencies
	require 'rack'
	require 'tilt'
	require 'rack/protection'
	require 'mustermann'
	require 'mustermann/sinatra'
	require 'mustermann/regular'

	s.add_dependency 'rack', '~> 2.2'
	s.add_dependency 'tilt', '~> 2.0'
	s.add_dependency 'rack-protection', version
	s.add_dependency 'mustermann', '~> 1.0'

	ruby2_keywords method_name
	ruby2_keywords(method_name) if respond_to?(:ruby2_keywords, true)

Fix Delegator to pass keyword arguments for Ruby 3.0 #1684

Fix Delegator to pass keyword arguments for Ruby 3.0 #1684

Conversation

andrewtblake commented Mar 19, 2021

dentarg commented Mar 20, 2021

andrewtblake commented Mar 20, 2021 via email

andrewtblake commented Mar 20, 2021 via email

dentarg commented Mar 22, 2021

andrewtblake commented Mar 22, 2021 via email

andrewtblake commented Mar 22, 2021 via email

dentarg left a comment

Choose a reason for hiding this comment

dentarg Mar 23, 2021

Choose a reason for hiding this comment

dentarg Mar 23, 2021

Choose a reason for hiding this comment

dentarg Mar 23, 2021

Choose a reason for hiding this comment

andrewtblake commented Mar 23, 2021 via email

dentarg Mar 25, 2021

Choose a reason for hiding this comment

dentarg commented Mar 25, 2021

jkowens commented Mar 25, 2021