Skip to content

handle ruby 3.x keyword arguments delegation#53

Merged
peerkleio merged 5 commits into
clio:masterfrom
rcugut:ruby-3-support
Jun 9, 2023
Merged

handle ruby 3.x keyword arguments delegation#53
peerkleio merged 5 commits into
clio:masterfrom
rcugut:ruby-3-support

Conversation

@rcugut

@rcugut rcugut commented Jan 6, 2022

Copy link
Copy Markdown
Contributor

This adds code compatibility with ruby 3.x changes in regard the delegation of positional splat arguments.

More info in the official ruby blog post explaining this change:
https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

The existing code breaks for ruby 3.x:

ArgumentError: wrong number of arguments (given 3, expected 1..2)
# ./gems/activerecord-6.1.4.4/lib/active_record/associations.rb:1457:in `has_many'
# ./lib/polymorphic_integer_type/extensions.rb:90:in `has_many'

I tested (locally) and the code changes are backward compatible for ruby 2.6 and 2.7.

@rcugut

rcugut commented Jan 6, 2022

Copy link
Copy Markdown
Contributor Author

PS: I see the CI Github Actions setup is "hardcoded" to use ruby 2.6

- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: "2.6"

@olegantonyan olegantonyan mentioned this pull request Feb 3, 2022
@olegantonyan

Copy link
Copy Markdown
Contributor

Thank you @rcugut This already works in production, but I also had to change few things to make make it work in Rails 7. Hope this will be merged soon

@rcugut

rcugut commented Feb 3, 2022

Copy link
Copy Markdown
Contributor Author

Thank you @rcugut This already works in production, but I also had to change few things to make make it work in Rails 7. Hope this will be merged soon

Thank you @olegantonyan

I just updated to Rail 7 too, and I had to only change the gemspec to for activerecord 7, and it works.

I'll watch for an update and use the published gem when you push it. Thank you

@olegantonyan

Copy link
Copy Markdown
Contributor

If only I was a contributor here, he-he :)
For Rails 7 I've changed this olegantonyan@c01f473 b/c otherwise it broke non-integer polymorphic association. Also, it doesn't work with STI (when has_many points to model using STI). It's not crucial for me, but maybe someone is already working on proper fix for this

@peerkleio
peerkleio merged commit 03d89f9 into clio:master Jun 9, 2023
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.

3 participants