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

Inject comments from .thrift into generated source #218

Open
jamesbroadhead opened this issue Feb 5, 2016 · 6 comments
Open

Inject comments from .thrift into generated source #218

jamesbroadhead opened this issue Feb 5, 2016 · 6 comments

Comments

@jamesbroadhead
Copy link

Often, thrift definitions have useful comments, which are not immediately visible if navigating from code to generated source.

It could be helpful if scrooge generated javadoc-style comments from comments in thrift definitions and injected them into the generated source to help in cases like this.

@mosesn
Copy link
Contributor

mosesn commented Feb 5, 2016

@jamesbroadhead I think we do, actually. See the gold file output. This is true across all of the kinds of code we generate, as far as I know.

@kevinoliver
Copy link
Contributor

@mosesn My guess is that there are places where we miss injecting them.

@jamesbroadhead It'd be great if you could enumerate those places where it would help.

@kuroneko25
Copy link

kuroneko25 commented Jun 27, 2016

We are seeing a similar problem where comments in thrift definitions are not included in the generated java source code.

We are using the scrooge maven plugin:

          <groupId>com.twitter</groupId>
          <artifactId>scrooge-maven-plugin</artifactId>
          <version>3.14.1</version>
          <configuration>
            <thriftSourceRoot>${basedir}/../thrift</thriftSourceRoot>
            <language>java</language>
            <thriftOpts>
              <!-- add other Scrooge command line options using thriftOpts -->
              <thriftOpt>--finagle</thriftOpt>
            </thriftOpts>
            <!-- tell scrooge to not to build the extracted thrift files (defaults to true) -->
            <buildExtractedThrift>false</buildExtractedThrift>
          </configuration>

And our thrift definition looks something like this:

...
/**
 * javadoc comment
 */
struct MyStruct {
  /* javadoc comment */
  1: string data;
}

service MyService {
  /**
   * javadoc comment
   */
  MyStruct myMethod(
    1: MyStruct input)
}
...

but none of the javadoc is included in the generated files.

@mosesn
Copy link
Contributor

mosesn commented Jun 27, 2016

@kuroneko25 I don't think we support javadoc-style (two stars at the beginning), I think we only support c-style (one star at the beginning). Can you try again like so:

...
/*
 * javadoc comment
 */
struct MyStruct {
  /* javadoc comment */
  1: string data;
}

service MyService {
  /*
   * javadoc comment
   */
  MyStruct myMethod(
    1: MyStruct input)
}
...

It's definitely weird that the one inside the struct doesn't work. We should look into that.

@kevinoliver
Copy link
Contributor

Moses, I think its only Doc-comment style (/** */) that is converted.

But it looks like we don't do this for the generated Java code. Note that
GoldService.java
https://github.com/twitter/scrooge/blob/develop/scrooge-generator-tests/src/test/resources/gold_file_output_java/com/twitter/scrooge/test/gold/thriftjava/GoldService.java
does
not have the "Hello, I'm a comment" comment that the Scala one has
https://github.com/twitter/scrooge/blob/develop/scrooge-generator-tests/src/test/resources/gold_file_output_scala/com/twitter/scrooge/test/gold/thriftscala/GoldService.scala#L32-L36
.

A PR to add this for the Java generator would be gladly accepted.

On Mon, Jun 27, 2016 at 1:52 PM, Moses Nakamura notifications@github.com
wrote:

@kuroneko25 https://github.com/kuroneko25 I don't think we support
javadoc-style (two stars at the beginning), I think we only support c-style
(one star at the beginning). Can you try again like so:

...
/*

  • javadoc comment
    /
    struct MyStruct {
    /
    javadoc comment */
    1: string data;
    }

service MyService {
/*

  • javadoc comment
    */
    MyStruct myMethod(
    1: MyStruct input)
    }
    ...

It's definitely weird that the one inside the struct doesn't work. We
should look into that.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#218 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAVMXUHspOJNa3BQldNEdpNiJYtzl6S3ks5qQDfzgaJpZM4HUO2E
.

Kevin Oliver | Twitter, Inc | follow me: @kevino http://twitter.com/kevino

@mosesn
Copy link
Contributor

mosesn commented Jun 27, 2016

Whoops, my bad! I think in mainstream thrift, they only do /* */, we should look into supporting that too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants