search

dry-rb / dry-transaction

Business transaction DSL
http://dry-rb.org/gems/dry-transaction
MIT License
469 stars 55 forks source link

Restore broadcast of output value for step_succeeded events (and reorder broadcast args) #88

Closed timriley closed 6 years ago

timriley commented 6 years ago

This PR makes 2 changes:

  1. It restores the broadcast of the output value on step_succeeded events. This was (inadvertently?) removed in #83, with this change to rename the broadcast event from #{step_name}_succeeded to step_succeeded.
  2. It makes the broadcast arguments consistent across both the step_succeeded and step_failed events, and moves the output/failure value to be the second argument in the broadcast event, right after step name, with the input args last, since they are variadic/splatted, which would potentially make finding the output value harder to do.

With this change made, subscribers should now have all the information they need, but I wonder if we can make it even clearer by publishing a hash of data instead of these positional args. So instead of this:

broadcast :step_succeeded, step_name, value, *args

We do this:

broadcast :step_succeeded, step: step_name, result: value, args: args

Or possibly even this (if we want to separate the single input value from the rest of the step args, which'd be a nice distinction to make, I think):

broadcast :step_succeeded, step: step_name, result: value, input: args.first, args: args[1..-1]

I'm going to hold up the next release until we can clear this up, since I don't want to have to change the API for the broadcast events twice in quick succession.

timriley commented 6 years ago

@GustavoCaso @flash-gordon @solnic Do you have any thoughts on how we deal with these broadcast event args? I'm inclined to opt for the 3rd option:

broadcast :step_succeeded, step: step_name, result: value, input: args.first, args: args[1..-1]

I'll try make this change Wednesday or Thursday this week (and then push out a release) unless you have any other thoughts.

GustavoCaso commented 6 years ago

@timriley thank you for this.

Yes, I agree on sending a hash would be easier for the user to extract all the information they need.

Regarding the different options, I'm with you on using the 3rd option.

broadcast :step_succeeded, step: step_name, result: value, input: args.first, args: args[1..-1]
flash-gordon commented 6 years ago

@timriley right, passing a hash is better 👍

timriley commented 6 years ago

Looks like @GustavoCaso has fully covered this in #90, so I'll close this.