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

rom-rb / rom

Data mapping and persistence toolkit for Ruby

https://rom-rb.org

MIT License

2.07k stars 161 forks source link

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

Open dgollahon opened 9 months ago

dgollahon commented 9 months ago

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 commented 9 months ago

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.

flash-gordon commented 9 months ago

I think we can drop ruby 2.x safely

katafrakt commented 5 months ago

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

dgollahon commented 5 months ago

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.

solnic commented 5 months ago

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

solnic commented 5 months ago

I think we can drop ruby 2.x safely

Yes let's do this

dgollahon commented 5 months ago

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

Correct, when combined with #combine.

solnic commented 5 months ago

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