-
Notifications
You must be signed in to change notification settings - Fork 1k
Removed references to deprecated onSuccess and onFailure methods. #1051
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.
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
Conversation
_overviews/core/futures.md
Outdated
case npe: NullPointerException => | ||
println("I'd be amazed if this printed out.") | ||
} | ||
The `failed.foreach` callback is only executed if the future fails, that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's technically true, but doesn't really explain what's going on. It'd be more accurate to say that future.failed
converts the Future[Failed[Throwable]]
to a Future[Success[Throwable]]
to allow foreach
to operate on the Throwable
, and will produce a Future[Failure[NoSuchElementException]]
if the original future succeeded, essentially swapping the position of the throwable.
I think probably the wording can just be a little simpler though, especially as there's a link to a projections anchor further down the page.
"To handle failed results, you can first use the failed
projection to convert the Failure[Throwable]
to a Success[Throwable]
and then use foreach
on the newly-successful Future
instead."
I do note that the projections section clarifies that the failed
projection is to enable handling exceptions with for comprehensions, though, so I wonder if there's actually an advantage to future.failed.foreach
over simply
future onComplete {
case Success(_) =>
case Failure(t) => println("Uh oh")
}
given that the former requires creating a new Future
. Maybe it's not worth mentioning the side-effect-on-failure-only scenario at all until the projections section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that failed.foreach
doesn't really add very much (indeed if this was particularly useful then I expect onFailure
wouldn't be deprecated).
So how about we just remove this failure-scenario discussion/example, replace it with your "to handle failed results" paragraph and a link to the 'Projections' section where it can be properly explained?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so I've went a bit further to try and make things immediately obvious if the link is followed. See what you think..
_overviews/core/futures.md
Outdated
be applied to a `Throwable`. The following special exceptions are | ||
the `failed` projection method, which allows this `Throwable` to be | ||
treated as the success value of another `Future`. | ||
The following special exceptions are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you were trying to avoid touching the next line unnecessarily but it's made the hard-wrapping a little untidy.
LGTM, perhaps @viktorklang would like to take a quick look |
thank you, Ian! |
I've tried to provide the equivalent examples and discussion; removed a couple of paragraphs that no longer seemed quite relevant (notably the little explanation of partial functions).
Hope this is okay.