Skip to content

Generated annotation#1539

Merged
raboof merged 6 commits intoakka:mainfrom
sebastian-alfers:generated-annotation
Jan 12, 2022
Merged

Generated annotation#1539
raboof merged 6 commits intoakka:mainfrom
sebastian-alfers:generated-annotation

Conversation

@sebastian-alfers
Copy link
Copy Markdown
Contributor

@sebastian-alfers sebastian-alfers commented Jan 3, 2022

Open questions:

  • Should Java's static class also get this annotation?
  • Should Scala's object also get this annotation?

Fixes #1538

@raboof
Copy link
Copy Markdown
Contributor

raboof commented Jan 4, 2022

* [ ]  Should Java's `static class` also get this annotation?
* [ ]  Should  Scala's `object` also get this annotation?

I guess all generated classes should get the annotation? Any reason why not?

@sebastian-alfers
Copy link
Copy Markdown
Contributor Author

Any reason why not?

Nothing I am aware of, thanks for confirming :-)

Copy link
Copy Markdown
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Maybe also ScalaServer/PowerApiTrait.scala.txt?

Otherwise LGTM. Would be good to actually test with JaCoCo, but I'd also be OK with merging and testing against the published snapshot

return new Default@{service.name}Client(settings, sys);
}

@@AkkaGrpcGenerated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is still necessary since it's an inner class of a class that is already annotated with AkkaGrpcGenerated, but cant hurt ;)

@sebastian-alfers
Copy link
Copy Markdown
Contributor Author

Added the annotation for all traits and tested locally with JaCoCo - all good.

Now the only generated classes that are still in the report are based on the scalapb codegen which we do not control.

@raboof
Copy link
Copy Markdown
Contributor

raboof commented Jan 6, 2022

Added the annotation for all traits and tested locally with JaCoCo - all good.

Great!

Now the only generated classes that are still in the report are based on the scalapb codegen which we do not control.

In some cases we might be able to tweak via https://scalapb.github.io/docs/customizations/#adding-annotations - but perhaps that's a 'next step'

@sebastian-alfers
Copy link
Copy Markdown
Contributor Author

Maybe I misunderstood this but I think this has to be added directly into the proto files so that should be done by the end-user?!

@raboof
Copy link
Copy Markdown
Contributor

raboof commented Jan 6, 2022

Maybe I misunderstood this but I think this has to be added directly into the proto files so that should be done by the end-user?!

Yes, I think so, too. That might be fine, since it's the user who wants to run JaCoCo and optimize the reporting?

@sebastian-alfers
Copy link
Copy Markdown
Contributor Author

@raboof Anything else you would like to see here? Or is this good to go?

Copy link
Copy Markdown
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Thanks for the ping, looks good now indeed!

@raboof raboof merged commit 756a185 into akka:main Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce @AkkaGrpcGenerated annotation to generate classes

2 participants