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

Fix usage of inherited Sinatra::Base classes keyword arguments #1670

Merged

Conversation

duduribeiro
Copy link
Contributor

@duduribeiro duduribeiro commented Dec 17, 2020

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.
@duduribeiro duduribeiro force-pushed the fix_positional_arguments_in_ruby_3 branch from b821d91 to 64347b0 Compare December 17, 2020 18:49
@dentarg
Copy link
Member

dentarg commented Dec 17, 2020

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

@duduribeiro
Copy link
Contributor Author

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

Thanks 🍻

@duduribeiro
Copy link
Contributor Author

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

Thanks

.travis.yml Outdated
@@ -1,7 +1,7 @@
---
language: ruby

dist: trusty
dist: xenial
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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).

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Switching to Github Actions would be 💯

@jkowens jkowens mentioned this pull request Dec 29, 2020
@jkowens
Copy link
Member

jkowens commented Dec 29, 2020

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

@duduribeiro
Copy link
Contributor Author

@jkowens thanks!

This PR is ready for review now 🍻

lib/sinatra/base.rb Outdated Show resolved Hide resolved
@jkowens
Copy link
Member

jkowens commented Dec 30, 2020

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

Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@namusyaka namusyaka merged commit d39b135 into sinatra:master Jan 7, 2021
@namusyaka namusyaka added this to the v2.1.1 milestone Jan 7, 2021
@duduribeiro duduribeiro deleted the fix_positional_arguments_in_ruby_3 branch January 7, 2021 15:16
@duduribeiro duduribeiro restored the fix_positional_arguments_in_ruby_3 branch January 7, 2021 15:27
dentarg added a commit to dentarg/sinatra that referenced this pull request Feb 16, 2022
jkowens pushed a commit that referenced this pull request Apr 14, 2022
jkowens pushed a commit that referenced this pull request Apr 14, 2022
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.

Incompatibility with Ruby 3 when extending Sinatra::Base
4 participants