-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attempting to solve the Ruby 2.7 Kwarg warnings/3.0 issue with kwargs in unsafe/bang methods #94
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Helpful to switch versions to see different outputs
…method Under Ruby 2.7.8, running the specs will produce these warnings: /<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 Under Ruby 3.2.2, running the specs will produce these 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'
Produces this issue in both Ruby 2.7.8 and Ruby 3.2.2 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'
Needed so we keep the "if last argument is a boolean, it controls the state machine :action being performed" behavior working, as documented in machine.rb (look for `action` and `skip` in the docs) Curiously, in line 238, - using *args makes all tests pass - using *pargs keeps the same test as before failing What am I doing wrong?
4459d9d
to
49c711f
Compare
@seuros rebased! |
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 ? |
I will take over this branch and ping you in the new PR |
seuros
pushed a commit
that referenced
this pull request
Jun 27, 2023
… in unsafe/bang methods (#94)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some time ago I opened this issue: #82
It was closed because everything seemed to be fine.
However #82 (comment) 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
And in Ruby 3.2.2 I would get straight errors:
So there's a breaking issue here.
In 746d544 I attempted a basic splitting of the arguments and do the call.
This solved the ERRORS above, but made another test fail:
By looking
machine.rb
and searching forskip
andaction
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 theaction
defined in thestate_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:
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.Machine#define_helper
instead?Thanks