rom icon indicating copy to clipboard operation
rom copied to clipboard

Fix `command(:create, result: :many)` for combines

Open dgollahon opened this issue 9 months ago • 8 comments

  • 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)).

dgollahon avatar Sep 28 '23 06:09 dgollahon

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.

dgollahon avatar Sep 28 '23 06:09 dgollahon

I think we can drop ruby 2.x safely

flash-gordon avatar Sep 28 '23 07:09 flash-gordon

I see 2.x was already dropped from rom-sql. So should we do the same here? I can create the PR if needed.

katafrakt avatar Jan 26 '24 10:01 katafrakt

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.

dgollahon avatar Jan 31 '24 00:01 dgollahon

Thanks for the PR! So you're saying that without this, it's broken under 3.x?

solnic avatar Feb 01 '24 16:02 solnic

I think we can drop ruby 2.x safely

Yes let's do this

solnic avatar Feb 01 '24 16:02 solnic

Thanks for the PR! So you're saying that without this, it's broken under 3.x?

Correct, when combined with #combine.

dgollahon avatar Feb 05 '24 22:02 dgollahon

How about we just ditch 2.x and fix signatures to be command(type, **opts)?

solnic avatar Feb 06 '24 04:02 solnic