Fix usage of inherited Sinatra::Base classes keyword arguments by duduribeiro · Pull Request #1670 · sinatra/sinatra

duduribeiro · 2020-12-17T18:45:37Z

Closes #1669
Using keyword arguments on inherited classes from Sinatra::Base breaks
with ArgumentError: wrong number of arguments (given X, expected 0) on Ruby 3

This can be reproduced by this minimal example:

app.rb

require 'sinatra'

class KWApp < Sinatra::Base
  def initialize(app:, comp:)
    p app
    p comp

    app.get '/' do
      "Hello from KWApp"
    end

    super(app)
  end
end

class PApp < Sinatra::Base
  def initialize(app, comp)
    p app
    p comp

    app.get '/' do
      "Hello from PApp"
    end

    super(app)
  end
end

config.ru

require "./app"
require "sinatra"

map "/" do
    run KWApp.new(app: Sinatra.new, comp: 2)
end

map "/p" do
    run PApp.new(Sinatra.new, 2)
end

This example works on ruby < 3 but it breaks when using Ruby 3 with:

 KWApp.new(app: 1, comp: 2)
ArgumentError: wrong number of arguments (given 1, expected 0; required keywords: app, comp)
from (pry):3:in `initialize

This commit fixes the usage of it on Ruby3 by allowing the method to
accept both positional and keyword arguments.

Using keyword arguments on inherited classes from Sinatra::Base breaks with ArgumentError: wrong number of arguments (given X, expected 0). This commit fixes the usage of it on Ruby3 by allowing the method to accept both positional and keyword arguments.

dentarg · 2020-12-17T19:23:45Z

@duduribeiro Nice! Do you think you could add your example as a test?

duduribeiro · 2020-12-17T19:24:38Z

@dentarg yeah!. Will do it and ping you after.

Thanks 🍻

duduribeiro · 2020-12-17T19:36:50Z

hey @dentarg,
I've added a test of it. Can u check if it is ok?

Thanks

duduribeiro · 2020-12-18T13:24:03Z

.travis.yml

 language: ruby

-dist: trusty
+dist: xenial


On trusty rvm is failing to install ruby 3.0.0-preview1, but upgrading to xenial makes the jruby build fails.

I think we should try to get the test suite running on GitHub Actions

@dentarg wanna merge this and work on GH actions after? I can work on this in a new PR if I get access on it.

Also, this will be importante because:
https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing

Building on a public repositories only We love our OSS teams who choose to build and test using TravisCI and we fully want to support that community. However, in recent months we have encountered significant abuse of the intention of this offering (increased activity of cryptocurrency miners, TOR nodes operators etc.). Abusers have been tying up our build queues and causing performance reductions for everyone. In order to bring the rules back to fair playing grounds, we are implementing some changes for our public build repositories. For those of you who have been building on public repositories (on travis-ci.com, with no paid subscription), we will upgrade you to our trial (free) plan with a 10K credit allotment (which allows around 1000 minutes in a Linux environment).

@duduribeiro I'm not maintainer or collaborator, just a fellow contributor :-) It is @namusyaka or @jkowens that can merge PRs

I think Actions should work in your fork of Sinatra, so you can work on it right now if you want

Switching to Github Actions would be 💯

jkowens · 2020-12-29T18:02:27Z

I've worked out the Travis issues. Should be good if you rebase master. Thanks!

duduribeiro · 2020-12-29T18:59:34Z

@jkowens thanks!

This PR is ready for review now 🍻

lib/sinatra/base.rb

jkowens · 2020-12-30T18:29:57Z

This looks good to me! @namusyaka can you take a look as well?

namusyaka

Looks good, thanks.

Follows the "The Only Correct Way" from https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html Related to sinatra#1670 Fixes sinatra#1749

Follows the "The Only Correct Way" from https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html Related to #1670 Fixes #1749

duduribeiro force-pushed the fix_positional_arguments_in_ruby_3 branch from b821d91 to 64347b0 Compare December 17, 2020 18:49

Include tests

06e049c

duduribeiro added 4 commits December 17, 2020 19:07

Import rvm Key on travis pipeline

561ec54

Trying to use ruby-head instead of ruby-3.0.0-preview1 to avoid errors

be56991

Returning ruby-3.0.0 to test it on xenial linux

3c80f4f

Upgrade to Xenial

a5ed7fb

duduribeiro commented Dec 18, 2020

View reviewed changes

jkowens mentioned this pull request Dec 29, 2020

Add Ruby 3 to CI #1673

Merged

Merge branch 'master' into fix_positional_arguments_in_ruby_3

605d7bf

duduribeiro requested a review from jkowens December 29, 2020 18:59

jkowens reviewed Dec 29, 2020

View reviewed changes

lib/sinatra/base.rb Outdated Show resolved Hide resolved

duduribeiro added 3 commits December 29, 2020 18:58

Apply suggestions from code review

b979ca6

Change initialize method to accept kwargs

2760659

Merge branch 'master' into fix_positional_arguments_in_ruby_3

77e6872

namusyaka approved these changes Jan 7, 2021

View reviewed changes

namusyaka merged commit d39b135 into sinatra:master Jan 7, 2021

namusyaka added this to the v2.1.1 milestone Jan 7, 2021

duduribeiro deleted the fix_positional_arguments_in_ruby_3 branch January 7, 2021 15:16

duduribeiro restored the fix_positional_arguments_in_ruby_3 branch January 7, 2021 15:27

This was referenced Feb 16, 2022

An unexpected error occurred! {:error=>#<ArgumentError: wrong number of arguments (given 3, expected 1..2)> elastic/logstash#13777

Closed

2.2.0 regression in JRuby handling of inherited Sinatra::Base keyword args #1749

Closed

dentarg added a commit to dentarg/sinatra that referenced this pull request Feb 16, 2022

Use ruby2_keywords for delegation

e814c6f

Follows the "The Only Correct Way" from https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html Related to sinatra#1670 Fixes sinatra#1749

dentarg mentioned this pull request Feb 16, 2022

Use ruby2_keywords for delegation #1750

Merged

jkowens pushed a commit that referenced this pull request Apr 14, 2022

Use ruby2_keywords for delegation

88e2148

Follows the "The Only Correct Way" from https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html Related to #1670 Fixes #1749

jkowens pushed a commit that referenced this pull request Apr 14, 2022

Use ruby2_keywords for delegation

3dd160b

Follows the "The Only Correct Way" from https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html Related to #1670 Fixes #1749

Conversation

duduribeiro commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

app.rb

config.ru

Uh oh!

dentarg commented Dec 17, 2020

Uh oh!

duduribeiro commented Dec 17, 2020

Uh oh!

duduribeiro commented Dec 17, 2020

Uh oh!

duduribeiro Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

dentarg Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

duduribeiro Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

dentarg Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

dentarg Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

jkowens Dec 29, 2020

Choose a reason for hiding this comment

Uh oh!

jkowens commented Dec 29, 2020

Uh oh!

duduribeiro commented Dec 29, 2020

Uh oh!

Uh oh!

jkowens commented Dec 30, 2020

Uh oh!

namusyaka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

duduribeiro commented Dec 17, 2020 •

edited

Loading