**kwargs extravaganza #439
Conversation
There was a problem hiding this comment.
Thanks, @adamruzicka, the changes look logical, but I don't understand why it raises
NoMethodError: undefined method 'to_hash' for Support::DummyExample::Polling:Class. It seems Support::DummyExample::Polling is Action, which should (?) respond to_hash via recursive_to_hash (?). Obviously something gets broken on a way to serialization, but no idea when.
lib/dynflow/delayed_plan.rb
Outdated
| @@ -70,7 +72,8 @@ def args | |||
|
|
|||
| # @api private | |||
| def self.new_from_hash(world, hash, *args) | |||
| serializer = Utils.constantize(hash[:args_serializer]).new(nil, hash[:serialized_args]) | |||
| puts hash | |||
adamruzicka
force-pushed
the
kwargs-extravaganza
branch
from
e76e150 to
34118fc
Compare
Apparently in https://github.com/Dynflow/dynflow/blob/master/lib/dynflow/execution_plan.rb#L287 the last |
adamruzicka
force-pushed
the
kwargs-extravaganza
branch
from
b752872 to
2ecdca8
Compare
|
Seems okay-ish?.. I mean, do we need to pass |
|
I'm afraid so. In some place we explicitly need to tell ruby to not treat the previous argument as an options hash (and not call to_hash on it) :/ |
good enough to make examples/future_execution and examples/orchestrate_evented work
by working around https://bugs.ruby-lang.org/issues/14909
Concurrent: [DEPRECATED] Method #disable_at_exit_handlers! has no effect since it is no longer needed, see ruby-concurrency/concurrent-ruby#841
adamruzicka
force-pushed
the
kwargs-extravaganza
branch
from
f55ede0 to
cee313e
Compare
There was a problem hiding this comment.
Thanks, @adamruzicka, I didn't find any issues now, you did incredible work here :D
Just one this inline and I guess it's ready.
| @@ -43,7 +43,7 @@ def start | |||
| def clean! | |||
| plans = @world.persistence.find_old_execution_plans(Time.now.utc - @max_age) | |||
| report(plans) | |||
| @world.persistence.delete_execution_plans(uuid: plans.map(&:id)) | |||
| @world.persistence.delete_execution_plans(uuid: plans.map(&:id), **{}) | |||
There was a problem hiding this comment.
Should we have **{} here? I mean, delete_execution_plans looks to have def delete_execution_plans(filters, batch_size = 1000, backup_dir = nil). It won't hurt, but it just seems unnecessary. On the other hand, I've noticed this: https://github.com/Dynflow/dynflow/blob/master/lib/dynflow/debug/telemetry/persistence.rb#L34-L38. I guess it makes more sense to add **kwargs support there just in case.
|
In the end we decided to not go down this path. |
good enough to make examples/future_execution and examples/orchestrate_evented work
Let's see how many tests will be broken by this