[WIP]: Fix code generation problems for Play #524
Conversation
Release 0.5.0 is not generating valid code. This fixes the templates to adapt them to the recent API changes.
|
Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged. For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask! |
| @@ -25,23 +27,25 @@ import io.grpc.Status; | |||
| * Abstract base class for implementing @{service.name} in Java and using as a play Router | |||
| */ | |||
| public abstract class Abstract@{service.name}Router extends PlayRouter implements @{service.name} { | |||
| private final Function<Throwable, Status> eHandler; | |||
| private final ActorSystem actorSystem; | |||
There was a problem hiding this comment.
Maybe it would be better to change PlayRouter to also requires an ActorSystem and not only a Materializer. But decide not to change it here to keep the impact of this PR small.
There was a problem hiding this comment.
I don't get this comment, because it has been changed to also require an actor system now.
There was a problem hiding this comment.
Sorry, I mean here, at the constructor:
Should it instead be:
@InternalApi abstract class PlayRouter(mat: Materializer, actorSystem: ActorSystem, serviceName: String) extends play.api.routing.Router?
In the end, user extending the generated code will need to inject an ActorSystem anyway.
| } | ||
|
|
||
| /** | ||
| * INTERNAL API | ||
| */ | ||
| final public scala.Function1<HttpRequest, Future<HttpResponse>> createHandler(String prefix, Materializer mat) { | ||
| return akka.grpc.internal.PlayRouterHelper.handlerFor( | ||
| @{service.name}HandlerFactory.create(this, prefix, mat, eHandler) | ||
| @{service.name}HandlerFactory.create(this, prefix, mat, eHandler, this.actorSystem) |
There was a problem hiding this comment.
It also makes more sense to have the exception handler as the last parameter, but again, avoid pilling up changes here.
| @@ -25,23 +27,25 @@ import io.grpc.Status; | |||
| * Abstract base class for implementing @{service.name} in Java and using as a play Router | |||
| */ | |||
| public abstract class Abstract@{service.name}Router extends PlayRouter implements @{service.name} { | |||
| private final Function<Throwable, Status> eHandler; | |||
| private final ActorSystem actorSystem; | |||
There was a problem hiding this comment.
I don't get this comment, because it has been changed to also require an actor system now.
|
OK TO TEST |
Thanks for reviewing, @johanandren. I will keep this as WIP until I have time to investigate why the tests for play-grpc are failing. |
|
@marcospereira I noticed akka-grpc was still depending on a RC of Play, added #531 to update. |
|
(looking at the conflicts now) |
|
Hmm, it seems like the changes in the PR were already part of #429 so are already on master - so this PR is perhaps no longer necessary? |
|
By the way, you can still use playframework/play-grpc#45 to see the tests failures. |
Release 0.5.0 is not generating valid code. This fixes the templates to adapt them to the recent API changes.
Fixes #523.
Status
WIP since the code now compiles, but the tests for play-grpc fail when using a local build of akka-grpc. To reproduce, just check out playframework/play-grpc#45 and run the tests.