Add a decorator for callback error handling

[sharmaayush358]

With the DecorateCallbackWithErrorHandling decorator, error handling will become easier for callbacks. This has been a pain-point for FSM users as they go with one of these options:

1. Not handle errors at all: This is because assigning the error to Event.Err is a non-idiomatic Go pattern.
2. Write Event.Err = err for each error case
3. Write their own decorator

Using this pattern, users can continue to write idiomatic Go code.

Test Plan

Wrote a unit test to verify the behaviour. It passed.
Existing tests also passed(verified by running make test).

[sharmaayush358]

@maxekman can you review this?

[coveralls]

coverage: 92.744% (+0.1%) from 92.644% when pulling d5d10e9 on sharmaayush358:ayush/callback-decorator into e668a85 on looplab:main.

[maxekman]

I like the general approach, but how do you feel about the function names? I think using Decorator is not the most common term in Go. Maybe you can look at some other code bases to see what they have named similar wrappers?

[sharmaayush358]

I've seen With getting used a lot. So we can simply remove the term Decorate and thus the name would be CallbackWithErrorHandling.
A common example of this is context.WithCancel.

[sharmaayush358]

@maxekman let me know if CallbackWithErrorHandling looks good to you. Or you can suggest an alternative if you have something in mind.

[sharmaayush358]

@maxekman @annismckenzie @mavogel ping on this

[annismckenzie]

I'm not sure why I'm mentioned. 😅 Haven't used this library in a while. Looking at the code, I fail to see why this would have to be added to upstream. You could just as well only do this for your project. Short of changing the Callback type to type Callback func(context.Context, *Event) error and adding this functionality natively, that is. What's your reasoning? I'd argue that keeping the error handling inside the callbacks actually makes more sense in this case because you'd otherwise have to deal with each error returned from a callback outside the state machine. What I'd like to see instead is more documentation on how to handle errors inside a chain of events and a bigger state machine so users can see how this is supposed to work. I know that checking e.Err and assigning to it is not the most idiomatic but it has a reason.

The downside of the code in this pull request lies in the fact that when you forget to wrap your callback in CallbackWithErrorHandling in your state machine but rely on this decorator to be there, bugs will inevitably creep in.

That is to say I'm conflicted on this. 😅 As for the naming, I'd go with CallbackWithError or WrapCallbackWithError instead.

[sharmaayush358]

I'm not sure why I'm mentioned. 😅 Haven't used this library in a while. Looking at the code, I fail to see why this would have to be added to upstream. You could just as well only do this for your project. Short of changing the Callback type to type Callback func(context.Context, *Event) error and adding this functionality natively, that is. What's your reasoning? I'd argue that keeping the error handling inside the callbacks actually makes more sense in this case because you'd otherwise have to deal with each error returned from a callback outside the state machine. What I'd like to see instead is more documentation on how to handle errors inside a chain of events and a bigger state machine so users can see how this is supposed to work. I know that checking e.Err and assigning to it is not the most idiomatic but it has a reason.

The downside of the code in this pull request lies in the fact that when you forget to wrap your callback in CallbackWithErrorHandling in your state machine but rely on this decorator to be there, bugs will inevitably creep in.

That is to say I'm conflicted on this. 😅 As for the naming, I'd go with CallbackWithError or WrapCallbackWithError instead.

I agree that changing the callback signature to type Callback func(context.Context, *Event) error is the most idiomatic solution. But the fact that it's backward incompatible diminishes its feasibility. Also, I second having better documentation since assigning to e.Err is highly unidiomatic Go. Given all this, the aim of this wrapper is to provide an option to the users of this library by the library itself since I suspect that this pattern would be being followed by a lot of users.

[annismckenzie]

I'm not sure why I'm mentioned. 😅 Haven't used this library in a while. Looking at the code, I fail to see why this would have to be added to upstream. You could just as well only do this for your project. Short of changing the Callback type to type Callback func(context.Context, *Event) error and adding this functionality natively, that is. What's your reasoning? I'd argue that keeping the error handling inside the callbacks actually makes more sense in this case because you'd otherwise have to deal with each error returned from a callback outside the state machine. What I'd like to see instead is more documentation on how to handle errors inside a chain of events and a bigger state machine so users can see how this is supposed to work. I know that checking e.Err and assigning to it is not the most idiomatic but it has a reason.
The downside of the code in this pull request lies in the fact that when you forget to wrap your callback in CallbackWithErrorHandling in your state machine but rely on this decorator to be there, bugs will inevitably creep in.
That is to say I'm conflicted on this. 😅 As for the naming, I'd go with CallbackWithError or WrapCallbackWithError instead.

I agree that changing the callback signature to type Callback func(context.Context, *Event) error is the most idiomatic solution. But the fact that it's backward incompatible diminishes its feasibility. Also, I second having better documentation since assigning to e.Err is highly unidiomatic Go. Given all this, the aim of this wrapper is to provide an option to the users of this library by the library itself since I suspect that this pattern would be being followed by a lot of users.

Agreed. I was merely suggesting to work on a better API before v3 is released – #86 is currently being worked on and will break backwards compatibility. Maybe hitch a ride alongside that? 🙃