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 Ruby 2.7 v 3 positional args #13

Merged
merged 1 commit into from
Feb 10, 2024
Merged

Conversation

jules2689
Copy link
Contributor

Ruby (https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/) deprecated positional arguments in 2.7, it is fully unsupported in Ruby 3.0

This PR fixes a use of this deprecated pattern which was blocking the rails installer command

@julik
Copy link
Contributor

julik commented Feb 10, 2024

Thank you for the change! Do I understand that Pecorino would simply not work for you on Ruby 2.7?

which was blocking the rails installer command

I don't want to be nitpicky but I would like to understand the problem better. I know kwargs changes have been a pain throughout the 2.x to 3.x lifecycle, but knowing how things fail exactly would help deepen my understanding. Could you paste the backtrace or the parse error that you encountered?

Also, should we add a CI run on 2.7 maybe? there is no reason for this gem to require 3.x

@julik
Copy link
Contributor

julik commented Feb 10, 2024

Ah I see:

/Users/julik/Code/libs/pecorino/lib/pecorino.rb:8:in `require_relative': /Users/julik/Code/libs/pecorino/lib/pecorino/throttle.rb:58: syntax error, unexpected ',' (SyntaxError)
...Pecorino::LeakyBucket.new(key:, **)
...                              ^
rake aborted!
Command failed with status (1)
/Users/julik/.rbenv/versions/2.7.7/bin/bundle:23:in `load'
/Users/julik/.rbenv/versions/2.7.7/bin/bundle:23:in `<main>'

Yes, let's fix this and also add 2.7 to CI. Thanks!

@julik julik self-requested a review February 10, 2024 22:50
@julik julik merged commit a7e3617 into cheddar-me:main Feb 10, 2024
1 check passed
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.

2 participants