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

Ruby 3.0.0 compatibility #2145

Merged
merged 4 commits into from
Dec 30, 2020
Merged

Ruby 3.0.0 compatibility #2145

merged 4 commits into from
Dec 30, 2020

Conversation

ericproulx
Copy link
Contributor

Just trying grape with ruby 3.0. I might have to update rubocop also

@anakinj
Copy link
Contributor

anakinj commented Dec 28, 2020

Before you waste any time in investigating failures on CI, the failing test related to arrays and XMLs is addressed in #2144

@grape-bot
Copy link

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#2145](https://github.com/ruby-grape/grape/pull/2145): Try ruby 3 0 - [@ericproulx](https://github.com/ericproulx).

Generated by 🚫 danger

@ericproulx ericproulx changed the title Try ruby 3 0 Ruby 3.0.0 compatibility Dec 28, 2020
@ericproulx
Copy link
Contributor Author

Fix #2142

@dblock
Copy link
Member

dblock commented Dec 29, 2020

We're moving to GH actions in #2143, which I just merged, update?

Add rails_6_1.gemfile

Add rails_6_1.gemfile

Transform **options into *options since Rack::Builder is calling middleware with .new(klass, *args, &block)

Add ** to versioned_path
Explicit ** for macro options

Replace ** by * since its anonymous

Add explicit { }
Transform **opts to *opts since its called with  with (*args, &block)

Add explicit **

Transform **opts to *opts since rspec-mock is calling .new(*args)

Add 6_1 gemfile

Add CHANGELOG

Explicitly require active_support/core_ext/array/conversions for the Array#to_xml method

Enable GitHub Actions with updated Rubocop and Danger

Merge master

Rubocop gemfiles

Explicitly require active_support/core_ext/array/conversions for the Array#to_xml method
@anakinj
Copy link
Contributor

anakinj commented Dec 29, 2020

This Danger failure is interesting. Think that Danger is not able to interact with it's old entires because it uses a different GitHub token than before. We could either live over the transitiontime or reuse the old token from TravisCI for the danger integration.

Maybe it also could work if a maintainer would delete the old danger entry created from the Travis run, ping @dblock

@ericproulx
Copy link
Contributor Author

Most of the work is :

  • Add { } or ** when needed
  • Change ** by *

The last one is quite particular. For the middlewares, I figured that Rack::Builder is instantiating them like this

middleware.new(app, *args, &block)

It's the same within the Grape::Middleware::Stack. Therefore, I replace the **options by *options and shifting the first argument if any.

Same thing happened with the lib/grape/validations/validators/base.rb. When having and_call_original, the class is actually instantiated :

original.call(*args, &block)

I haven't figured how to make a call properly when having a double splat param with a .new(*args)

eproulx@laptop grape % irb      
irb(main):001:0> def test(app, **options); end
=> :test
irb(main):002:0> args = [:app, {opt: [1,2]}]
=> [:app, {:opt=>[1, 2]}]
irb(main):003:0> test(*args)
Traceback (most recent call last):
        5: from /Users/eproulx/.rbenv/versions/3.0.0/bin/irb:23:in `<main>'
        4: from /Users/eproulx/.rbenv/versions/3.0.0/bin/irb:23:in `load'
        3: from /Users/eproulx/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/irb-1.3.0/exe/irb:11:in `<top (required)>'
        2: from (irb):3:in `<main>'
        1: from (irb):1:in `test'
ArgumentError (wrong number of arguments (given 2, expected 1))

That's why I made theses changes

@ericproulx
Copy link
Contributor Author

Instead of having **options we could go back to options = {} which will work.

@dblock
Copy link
Member

dblock commented Dec 30, 2020

I'm down with whatever solution. Can merge on green -needs a CHANGELOG. Looks like danger didn't rerun for the PR when it was updated? It should have picked up on the changelog. Did we configure this wrong?

@anakinj
Copy link
Contributor

anakinj commented Dec 30, 2020

I'm down with whatever solution. Can merge on green -needs a CHANGELOG. Looks like danger didn't rerun for the PR when it was updated? It should have picked up on the changelog. Did we configure this wrong?

Check my previous comment, think its because the token for danger has changed and it cannpt interact with previous entries

@dblock dblock merged commit 6b31f61 into ruby-grape:master Dec 30, 2020
@dblock
Copy link
Member

dblock commented Dec 30, 2020

I merged this, YOLO

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.

None yet

4 participants