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 issue with Ruby 3 keyword arguments #126

Merged
merged 1 commit into from Jul 18, 2022
Merged

Fix issue with Ruby 3 keyword arguments #126

merged 1 commit into from Jul 18, 2022

Conversation

jkowens
Copy link
Member

@jkowens jkowens commented Jul 17, 2022

@dentarg this seems to fix the issue with a failing spec in Ruby 3, is it the right way to fix this?

@jkowens
Copy link
Member Author

jkowens commented Jul 18, 2022

Going to merge, since this seems good for now.

@jkowens jkowens merged commit a518339 into master Jul 18, 2022
@@ -73,6 +73,7 @@ def self.translate(*types, &block)
Class.new(const_get(:NodeTranslator)) do
register(*types)
define_method(:translate, &block)
ruby2_keywords :translate
Copy link
Member

Choose a reason for hiding this comment

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

@jkowens I think you want the if too, otherwise Ruby 2.6 and lower won't be supported? http://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html, sinatra/sinatra#1750

Copy link
Member

@dentarg dentarg Jul 18, 2022

Choose a reason for hiding this comment

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

http://eregon.me/blog/2019/11/10/the-delegation-challenge-of-ruby27.html says ruby2_keywords is a no-op in Ruby 2.6 but I suspect it doesn't exist before that (I haven't checked though), so that's probably why CI doesn't complain

(Also haven't checked what min Ruby version is required by Sinatra/mustermann)

Copy link
Member Author

Choose a reason for hiding this comment

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

@dentarg ah shoot, I'm actually seeing warning messages now that I look. Maybe this isn't the right solution...this code is hard for me to follow 😅

Copy link
Member

Choose a reason for hiding this comment

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

Oh we don't need the if because we depend on the ruby2_keywords gem

s.add_runtime_dependency('ruby2_keywords', '~> 0.0.1')

@eregon
Copy link
Contributor

eregon commented Jul 19, 2022

FYI #130 is the proper fix.

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

3 participants