-
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.