Skip to content

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

Merged
merged 4 commits into from
May 19, 2018

Conversation

iwalt
Copy link
Contributor

@iwalt iwalt commented Apr 7, 2018

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.

case npe: NullPointerException =>
println("I'd be amazed if this printed out.")
}
The `failed.foreach` callback is only executed if the future fails, that
Copy link
Contributor

@giftig giftig Apr 8, 2018

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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

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
Copy link
Contributor

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.

@SethTisue
Copy link
Member

LGTM, perhaps @viktorklang would like to take a quick look

@SethTisue SethTisue merged commit aec3271 into scala:master May 19, 2018
@SethTisue
Copy link
Member

thank you, Ian!

rockpunk added a commit to rockpunk/docs.scala-lang that referenced this pull request Jun 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants