-
-
Notifications
You must be signed in to change notification settings - Fork 161
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 command(:create, result: :many)
for combines
#686
base: main
Are you sure you want to change the base?
Conversation
- In ruby 3.x the `result: :many` will get forwarded incorrectly if there is a `combine`. That is, something like `relation.command(:create, result: :many)` will work but `relation.combine(:child_relation).command(:create, result: :many)` will raise `ArgumentError: wrong number of arguments (given 2, expected 1)`. This fixes is it (I have tested by doing the following on my codebase `ROM::Relation::Combined.__send__(:ruby2_keywords, :command)`).
NOTE: I have not added any tests here because it was too much effort and I had too little time to figure out how to set up a local environment that worked. I would appreciate either someone else adding a test or pointers for getting set up. |
I think we can drop ruby 2.x safely |
I see 2.x was already dropped from rom-sql. So should we do the same here? I can create the PR if needed. |
To be clear, this isn't directly an issue of 2.x support. This doesn't work on 3.x, using 2.x style keywords happens to fix it. |
Thanks for the PR! So you're saying that without this, it's broken under 3.x? |
Yes let's do this |
Correct, when combined with |
How about we just ditch 2.x and fix signatures to be |
result: :many
will get forwarded incorrectly if there is acombine
. That is, something likerelation.command(:create, result: :many)
will work butrelation.combine(:child_relation).command(:create, result: :many)
will raiseArgumentError: wrong number of arguments (given 2, expected 1)
. This fixes is it (I have tested by doing the following on my codebaseROM::Relation::Combined.__send__(:ruby2_keywords, :command)
).