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.

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

Include Future Directives Examples for Java. #20619

Merged
merged 1 commit into from
May 30, 2016
Merged

Conversation

felipefzdz
Copy link
Contributor

This PR addresses the following directives for #20466

  • future-with-directives/onComplete.rst
  • future-with-directives/onSuccess.rst
  • future-with-directives/completeOrRecoverWith.rst

onComplete directive passes an Scala Try to the inner route. The lack of pattern matching in Java makes pretty complex the handling of that Try. I had to use func from scala-java8-compat, that's why I've included the package import commented out. In order to create a Partial Function I've used PFBuilder from akka.japi.pf package. That class is marked as EXPERIMENTAL, so I'm not sure if this is the intended way of consuming the result of onSuccess in Java.

That example gets really deep nested. As it needs to fit in the box generated by Sphinx I had to break the lines with not the best readability.

As my previous PR, the code here has been indented with tabs. It will be reformatted whenever the Java formatter will be ready.

@akka-ci
Copy link

akka-ci commented May 24, 2016

Can one of the repo owners verify this patch?

@ktoso
Copy link
Member

ktoso commented May 24, 2016

OK TO TEST

.matchAny(ex ->
complete(StatusCodes.InternalServerError(),
String.format("An error occurred: %s",
ex.getMessage())
Copy link
Member

Choose a reason for hiding this comment

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

that's pretty much the reason we I'm a fan of 2 spaces formatting (and we'll soon enforce that) :-)

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 with you. I've updated it to 2 spaces formatting and now is much more readable :)

@ktoso
Copy link
Member

ktoso commented May 24, 2016

Will review more in the morning, thanks for the PRs!

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels May 24, 2016
@akka-ci
Copy link

akka-ci commented May 24, 2016

Test PASSed.

@@ -0,0 +1,107 @@
/*
* Copyright (C) 2009-2016 Lightbend Inc. <http://www.lightbend.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR, the time here should be '2016-2016' which stands for file-created-year to file-modified-year :)

Copy link
Member

Choose a reason for hiding this comment

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

@Hawstein That should be nice too,just not that consistent,anyway ,we have git to track.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, just a little suggestion, not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting it @Hawstein. I've updated it!

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels May 25, 2016
@akka-ci
Copy link

akka-ci commented May 25, 2016

Test PASSed.

@akka-ci akka-ci removed the validating PR is currently being validated by Jenkins label May 25, 2016
// import static scala.compat.java8.JFunction.func;

final Route route = path(
PathMatchers.segment("divide")
Copy link
Member

Choose a reason for hiding this comment

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

Could we static import PathMatchers.* and make the path matching a single nice line?

Copy link
Member

Choose a reason for hiding this comment

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

And add a comment about importing that just like the func comment.

@johanandren
Copy link
Member

LGTM with a few nitpicks

@felipefzdz
Copy link
Contributor Author

Thanks @johanandren. I've amended the commit with your suggestions.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels May 29, 2016
@akka-ci
Copy link

akka-ci commented May 29, 2016

Test PASSed.

@ktoso
Copy link
Member

ktoso commented May 30, 2016

Thanks, excellent PR again @felipefzdz! To address your questions:

onComplete directive passes an Scala Try to the inner route. The lack of pattern matching in Java makes pretty complex the handling of that Try. I had to use func from scala-java8-compat, that's why I've included the package import commented out. In order to create a Partial Function I've used PFBuilder from akka.japi.pf package. That class is marked as EXPERIMENTAL, so I'm not sure if this is the intended way of consuming the result of onSuccess in Java.

Perhaps an interesting thing would be to add overloads in Java that handle the success/failure as 2 callbacks hm...

Yes, using the PFBuilder is OK here 👍

That example gets really deep nested. As it needs to fit in the box generated by Sphinx I had to break the lines with not the best readability.

Right, but looks pretty decent now, thanks!

@ktoso ktoso merged commit 2d6ec9c into akka:master May 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants