Attempting to solve the Ruby 2.7 Kwarg warnings/3.0 issue with kwargs in unsafe/bang methods

[julitrows]

Some time ago I opened this issue: https://github.com/state-machines/state_machines/issues/82

It was closed because everything seemed to be fine.

However https://github.com/state-machines/state_machines/issues/82#issuecomment-1569076464 by @jmanian made me think about the issue again. Instead of providing a test rails application that triggers the problem as I did when I opened the issue, this time I thought of getting inside the gem and try to solve it.

So I created some functional specs (see HybridCar and HybridCarTest) that would reflect the different ways of using positional and keyword arguments in bang methods, and see what happened.

In Ruby 2.7, I would get the Warnings of Doom™ when running the specs that use bang methods and keyword arguments:

• test_should_accept_keyword_argument_in_unsafe_method
• test_should_accept_positional_and_keyword_arguments_in_unsafe_method

/<my_path>/state_machines/lib/state_machines/event.rb:227: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/<my_path>/state_machines/test/files/models/hybrid_car.rb:32: warning: The called method `go_gas' is defined here
/<my_path>/state_machines/lib/state_machines/event.rb:227: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/<my_path>/state_machines/test/files/models/hybrid_car.rb:38: warning: The called method `go_back_in_time' is defined here

And in Ruby 3.2.2 I would get straight errors:

ERROR HybridCarTest#test_should_accept_keyword_argument_in_unsafe_method (0.03s)
Minitest::UnexpectedError:         ArgumentError: wrong number of arguments (given 1, expected 0; required keyword: driving_profile)
            /<my_path>/state_machines/test/files/models/hybrid_car.rb:31:in `go_gas'
            /<my_path>/state_machines/lib/state_machines/event.rb:227:in `block in add_actions'
            /<my_path>/state_machines/lib/state_machines/machine.rb:729:in `block (2 levels) in define_helper'
            /<my_path>/state_machines/test/functional/hybrid_car_test.rb:39:in `test_should_accept_keyword_argument_in_unsafe_method'

ERROR HybridCarTest#test_should_accept_positional_and_keyword_arguments_in_unsafe_method (0.06s)
Minitest::UnexpectedError:         ArgumentError: wrong number of arguments (given 2, expected 1; required keyword: driving_profile)
            /<my_path>/state_machines/test/files/models/hybrid_car.rb:37:in `go_back_in_time'
            /<my_path>/state_machines/lib/state_machines/event.rb:227:in `block in add_actions'
            /<my_path>/state_machines/lib/state_machines/machine.rb:729:in `block (2 levels) in define_helper'
            /<my_path>/state_machines/test/functional/hybrid_car_test.rb:46:in `test_should_accept_positional_and_keyword_arguments_in_unsafe_method'

So there's a breaking issue here.

In 746d5444cc780f5190d50af545ad3151b2a29804 I attempted a basic splitting of the arguments and do the call.

This solved the ERRORS above, but made another test fail:

FAIL VehicleUnsavedTest#test_should_allow_ignite_bang_with_skipped_action (0.04s)
        Expected false to be truthy.
        /<my_path>/state_machines/test/functional/vehicle_unsaved_test.rb:121:in `test_should_allow_ignite_bang_with_skipped_action'

By looking machine.rb and searching for skip and action I learned there's a somewhat obscure feature in which if you pass a boolean as last argument to an event method call, that boolean is used to determine if the action defined in the state_machine opening statement gets invoked or not.

I could not find in the code where this was happening (my metaruby reading is not that good), so I thought of preserving that last boolean argument. In the end, what I did was, if I could determine that there were no kwargs passed, I would just do the original call, instead of the one with positional and keyword arguments separated.

This makes all the tests pass.

I've got some questions tho:

1. Should the treatment I've given to that helper definition be applied to every other machine.define_helper call in the event.rb file? There are no other warnings under 2.7, but that might just be that there is no sufficient tests trying different parg/kwarg combinations.
2. If so, should this treatment happen in Machine#define_helper instead?

Thanks

[julitrows]

@seuros rebased!

[seuros]

I see a flaw in this implementation.

what will happen if i do

assert @hybrid_car.go_back_in_time!(1995, engine_options, flux_capacitor_settings)

where engine_options, flux_capacitor_settings are hashes ?

[seuros]

I will take over this branch and ping you in the new PR