Ruby 2.7 kwarg deprecation warnings on generated bang methods
julitrows opened this issue · 11 comments
Hello!
I use this gem along with the AR extension on a Rails app on Ruby 2.7. We override some of the transition methods for the state machine with kwargs, and we've started seeing the infamous Ruby 2.7 kwarg deprecation warnings when calling the unsafe versions of the transition methods.
# on the Car model with the state machine
def ignite(driver:)
update(driver: driver)
super()
endCalling something like car.ignite!(driver: current_user) from somewhere in the code will issue a warning:
/Users/julio/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/state_machines-0.5.0/lib/state_machines/event.rb:224: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/julio/code/state_machine_test/app/models/car.rb:9: warning: The called method `ignite' is defined here
I've created this small Rails app with some test code that reproduces the issue: https://github.com/julitrows/state_machine_test
Doing some diggning, it seems to go down to the StateMachines::Event class:
https://github.com/state-machines/state_machines/blob/master/lib/state_machines/event.rb#L224-L227
machine.define_helper(:instance, "#{qualified_name}!") do |machine, object, *args|
object.send(qualified_name, *args) || raise(StateMachines::InvalidTransition.new(object, machine, name))
endIt seems that object.send(qualified_name, *args) should be sending object.send(qualified_name, **args) in this particular case, but it probably would need to first detect what's *args (an array or a hash) before doing one or the other, to cover all cases.
NOTE I think this does not happen for the other helper definitions in that file.
I'm happy to send a PR to fix this, but I'm having trouble finding a test or tests in the gem codebase that would help reproduce the issue on the gem itself so I can start fixing.
Any help or pointers would be appreciated!
Thanks!
Oh cool. Thank you @bopm for the ping.
And thank you @julitrows for the reproduction repository.
I think your approach is good.
If you could open a PR, it will be helpful. I will be checking the other integration over the weekend for any other warning.
@seuros I am having bigger issue with after_update callback inside of state redefinition on Ruby 3.1 and Rails 7. I'll try to create at least a spec for reproducing that and bring it into a separate issue. Good to know that you are around for me to start trying that.
I was struggling to get non-flaky reproduction. Will try again now.
According to CI on master tests are running on Ruby 3
- https://github.com/state-machines/state_machines/blob/master/.github/workflows/ruby.yml
- https://github.com/state-machines/state_machines/actions/runs/4117695127/jobs/7157360165
So is this still an issue?
It not an issue anymore even the released versions are fine.
I plan to release master probably this weekend.
Great to hear, thank you @seuros !
I've just run into the exact issue described in the original post. It doesn't look like any changes have been made to support this use case. @julitrows how did you solve this? I'm considering changing the keyword argument to a positional argument as a quick solution.
@jmanian hi!
Where we would call an object.transition!(arg1: value1, arg2: value2) we ended up doing:
object.transition(arg1: value1, arg2: value2) || raise(StateMachines::InvalidTransition.new(*exception_args))Notice we're calling the safe method. This is what the gem does when generating the bang methods:
https://github.com/state-machines/state_machines/blob/master/lib/state_machines/event.rb#L224-L227
You can probably try to override the bang method somewhere in your code.
In our case, going back to positional argumentes was a no-no because we had too many and we preferred clarity. Also we don't like mixing up positional and keyword arguments.
Hope this helps you.