Add .tapFailure to TryOps #609
Conversation
- works basically like `tapError` on Task - used to avoid doing `failed.foreach` on task, which creates an unnecessary exception on Try.Success
| def tapFailure(action: Throwable => Unit): Try[A] = tr match { | ||
| case Success(_) => tr | ||
| case Failure(throwable) => | ||
| action(throwable) |
There was a problem hiding this comment.
What do you want to do when action(throwable) throws an exception? Not sure if current choice would be my default, but I'd like to hear your thoughts first. Please document your choice as well.
There was a problem hiding this comment.
Since this method was (in my mind) a replacement for .failed.foreach & setup(_.failed.foreach) I carried over the way it works there - which is just assuming that the code passed to tapFailure won't throw, and when it does, just throw it.
Now that I think about it though, it'd probably be more sensible to catch any error in tapFailure (since we're in the Try context, best to limit surprises) and return it to the user. I'm kind of on the fence whether it should be ignored instead, but since it's usually just going to be user error when it throws, it's better to know that it happened instead of failing silently.
LMK if this sounds sensible to you
There was a problem hiding this comment.
My thoughts were that any .tap method should never alter the result. The libraries are also on the fence: Task#tapError handles, Flow#wireTap I think too but Iterator#tapEach and Observable#doOnNext do not. I'd probably replicate Task approach and use the same name, but I'm okay with other behaviour if it's documented and tested.
tapErroron Taskfailed.foreachon task, which creates an unnecessary exception on Try.Success