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

GH-97 Add basic support for passing annotations to the generated Java stubs #171

Merged
merged 4 commits into from
May 6, 2019

Conversation

ilx
Copy link
Contributor

@ilx ilx commented Apr 23, 2019

Hi, we have a custom javadoc doclet that relies on some annotations to be present in Java files. In order to be able to use Scala for the code that needs to be processed with that doclet I have added very limited support that will pass annotations without any parameters to Java stubs generated by genjavadoc plugin. Can you please take a look and maybe merge it / improve it?

P.S. Are tests up-to-date? I'm not sbt user, so maybe I skipped something important but I could not get test task to pass:

[IJ]sbt:genjavadoc> {file:/Users/ilx/work/xebialabs/xlr-project/genjavadoc/}genjavadoc-plugin/test
[info] Updating genjavadoc-plugin...
[info] Done updating.
[info] Compiling 9 Scala sources to /Users/ilx/work/xebialabs/xlr-project/genjavadoc/plugin/target/scala-2.13.0-RC1/classes ...
[error] /Users/ilx/work/xebialabs/xlr-project/genjavadoc/plugin/src/main/scala/com/typesafe/genjavadoc/Plugin.scala:65:30: not found: type CompilationUnit
[error]     def newTransformer(unit: CompilationUnit) = new GenJavadocTransformer(unit)
[error]                              ^
[error] /Users/ilx/work/xebialabs/xlr-project/genjavadoc/plugin/src/main/scala/com/typesafe/genjavadoc/Plugin.scala:67:68: not found: type Transformer
[error]     class GenJavadocTransformer(val unit: CompilationUnit) extends Transformer with TransformCake {
[error]                                                                    ^
[error] /Users/ilx/work/xebialabs/xlr-project/genjavadoc/plugin/src/main/scala/com/typesafe/genjavadoc/Plugin.scala:67:43: not found: type CompilationUnit
[error]     class GenJavadocTransformer(val unit: CompilationUnit) extends Transformer with TransformCake {
[error]                                           ^
[error] /Users/ilx/work/xebialabs/xlr-project/genjavadoc/plugin/src/main/scala/com/typesafe/genjavadoc/Plugin.scala:65:49: type mismatch;
[error]  found   : com.typesafe.genjavadoc.GenJavadocPlugin.MyComponent.GenJavadocTransformer
[error]  required: com.typesafe.genjavadoc.GenJavadocPlugin.MyComponent.global.Transformer
[error]     def newTransformer(unit: CompilationUnit) = new GenJavadocTransformer(unit)

@lightbend-cla-validator
Copy link
Collaborator

Hi @ilx,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@ilx
Copy link
Contributor Author

ilx commented Apr 23, 2019

Just to inform you I have signed individual CLA

@raboof
Copy link
Contributor

raboof commented Apr 23, 2019

Hi ilx, thanks for your contribution! The tests should be up-to-date, as they're ran on Travis - I'll have a quick look if I can explain that error.

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

Might be good to add a test demonstrating the new feature?

def sig = pattern(s"$ret $name")
case class MethodInfo(access: String, pattern: String => String, ret: String, name: String, comment: Seq[String], d: Option[DefDef] = None) extends Templ {
def sig: String = {
s"""
Copy link
Contributor

Choose a reason for hiding this comment

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

The additional whitespace here makes tests fail


private object MyComponent extends PluginComponent with Transform {

import global._
Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently this imported CompilationUnit, perhaps best to bring it back?

… line as Java class stubs and write a simple test that demonstrates how to create Java stub with some annotation
@ilx
Copy link
Contributor Author

ilx commented Apr 23, 2019

@raboof tnx for the feedback - I've added an artificial scenario with @SupressWarnings annotation - arguments are ignored.

@ilx
Copy link
Contributor Author

ilx commented May 6, 2019

Hi, can someone take a look?

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

Sorry, I missed your update!

A couple of minor comments, but you can address those in a follow-up PR if you want. LGTM!

@@ -103,6 +103,36 @@ GenJavadoc can also be integrated into a Maven build (inspired by [this answer o
</profile>
~~~

You can integrate genjavadoc with gradle build:
Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Could've been a separate PR but 👍 :)

@@ -93,9 +108,27 @@ trait AST { this: TransformCake =>
}
}

case class MethodInfo(access: String, pattern: String => String, ret: String, name: String, comment: Seq[String]) extends Templ {
def sig = pattern(s"$ret $name")
case class MethodInfo(access: String, pattern: String => String, ret: String, name: String, comment: Seq[String], d: Option[DefDef] = None) extends Templ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might have been nice to only include the annotations here rather than the full DefDef?

} else {
annotations
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit simpler might be:

.map { a => s"@${a.symbol.fullName('.')}${System.lineSeparator}" }
.mkString

perhaps?

@raboof raboof merged commit 044f052 into lightbend:master May 6, 2019
@raboof raboof added this to the 0.14 milestone Sep 17, 2019
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