Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add .tapFailure to TryOps #609

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add .tapFailure to TryOps #609

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions core/src/main/scala/com/avsystem/commons/SharedExtensions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,19 @@ object SharedExtensionsUtils extends SharedExtensions {
*/
def toOptArg: OptArg[A] =
if (tr.isFailure) OptArg.Empty else OptArg(tr.get)

/**
* Apply side-effect only if Try is a failure.
*
* Don't use .failed projection here, because it unnecessarily creates Exception in case of Success,
* which is an expensive operation.
*/
def tapFailure(action: Throwable => Unit): Try[A] = tr match {
case Success(_) => tr
case Failure(throwable) =>
action(throwable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tr
}
}

class LazyTryOps[A](private val tr: () => Try[A]) extends AnyVal {
Expand Down
Loading