Skip to content

Conversation

@Legogris
Copy link
Contributor

@grape-bot
Copy link

grape-bot commented Oct 16, 2020

1 Warning
⚠️ There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 danger

@dblock dblock merged commit 552e01c into ruby-grape:master Oct 16, 2020
@dblock
Copy link
Member

dblock commented Oct 16, 2020

Thanks, I think this was the last one.

@Legogris
Copy link
Contributor Author

Thanks, I think this was the last one.

Thanks for merging! Almost ;)
#2121

@Legogris Legogris deleted the fix-2.7-deprecation-middleware-stack branch October 17, 2020 08:13
@Legogris Legogris mentioned this pull request Oct 19, 2020
@eregon
Copy link
Contributor

eregon commented Nov 16, 2020

This is unfortunately not correct, *args, **opts-delegation is not correct in some cases on 2.7.
The only correct way to do delegation on Ruby 2.7 is using ruby2_keywords + *args-delegation.
This is also what the Rails ActionDispatch::MiddlewareStack does (https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/stack.rb, which is very similar).

See https://eregon.me/blog/2019/11/10/the-delegation-challenge-of-ruby27.html for why it's broken on 2.7,
and as you mention above https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

A compatible delegation that works on Ruby 2.6, 2.7 and Ruby 3
In short: use Module#ruby2_keywords again.

FWIW, I'm coming from truffleruby/truffleruby#2159

@dblock
Copy link
Member

dblock commented Nov 16, 2020

Interesting. Can we get rid of all of this complexity?

@eregon
Copy link
Contributor

eregon commented Nov 17, 2020

Yes, if you use ruby2_keywords you only need a few ruby2_keywords calls on methods accepting *args.

@dblock
Copy link
Member

dblock commented Nov 17, 2020

@eregon care to PR a fix?

@eregon
Copy link
Contributor

eregon commented Nov 19, 2020

#2132

@Legogris
Copy link
Contributor Author

@eregon Derp, great that you caught this!

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.

4 participants