adomokos / light-service

Series of Actions with an emphasis on simplicity.
MIT License
837 stars 67 forks source link

Support for named argument in ruby 3 #224

Closed JeanSebTr closed 2 years ago

JeanSebTr commented 3 years ago

I've tried to update one of our project to ruby 3.0 and the only issue was in our services spec using ContextFactory.

We use named argument a lot and 3.0 break this when they're not passed explicitly using the double splat operator. ruby2_keywords marks the method as using the old style of named arguments. Calling it this way should works with older 2.x versions as well.

adomokos commented 3 years ago

Thank you! I'll add Ruby 3 to the GH actions to make sure this fixes what you're describing.

adomokos commented 3 years ago

@JeanSebTr - I looked into your PR today.

It seems ruby2_keywords was added to Ruby in 2.7. What if somebody (hopefully not 🤞 ) is using LS with Ruby <2.7? Could we add a guard clause for your statement?

Also, I'd love to see a unit test added for this change. Can you help me with that?

Thank you so much!

adomokos commented 3 years ago

I added Ruby 3.0.2 build in this PR. Your change might fix this.

JeanSebTr commented 3 years ago

It seems ruby2_keywords was added to Ruby in 2.7. What if somebody (hopefully not 🤞 ) is using LS with Ruby <2.7? Could we add a guard clause for your statement?

There is one: ruby2_keywords :with if RUBY_VERSION >= "2.7" as most ruby stuff, ruby2_keywords is a normal method and not a special keyword. I'm open to suggestions if it should take a different form than an if modifier.

I'll look into adding a test for it!

JeanSebTr commented 3 years ago

Your change might fix this.

looks like fixing that cop would require using (...) which would require dropping support for ruby < 2.7

JeanSebTr commented 2 years ago

Hi @adomokos !

I've rebased on main and added a spec covering this. I didn't manage to run them locally tho

JeanSebTr commented 2 years ago

Okay, managed to run specs locally, fixed the spec and made sure it fail on 3.0 without the fix.

alessandro-fazzi commented 2 years ago

Tried to suggest how to fix the CI in https://github.com/JeanSebTr/light-service/pull/2

On my repo it turned the CI green again.

alessandro-fazzi commented 2 years ago

Regarding the conversion about the backward compatibility, I'd like to leave this link https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/ as a reference into the PR.

JeanSebTr commented 2 years ago

Ok, sorry about all that back and forth. I've applied @adomokos suggestions and ran both rspec and rubocop locally. Re-tested as well without the fix and the spec does fail.

alessandro-fazzi commented 2 years ago

I already participated in the PR, deepened the matter, had the same need on a project and poor-man-tested the solution using 2.6 and 3.1: FWIW LGTM 🙂

adomokos commented 2 years ago

Do you guys need this released as a new version of the gem?

JeanSebTr commented 2 years ago

@adomokos On our side, we're not in a hurry to migrate to ruby 3, so I don't mind if you prefer waiting for other stuff to release a version.