Thrift
  1. Thrift
  2. THRIFT-179

Generated code should incorporate per-field doc strings

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Java - Compiler
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      Generated java code for Thrift structs should have doc strings from the .thrift file. The information is all there, it's just not made part of the generated code.

      In the default java generator, the doc strings would go on the field declarations. In the bean style generator, the the doc string would go on both the generated getters and setters.

      1. thrift-179.patch
        3 kB
        Bryan Duxbury
      2. thrift-179-v2.patch
        4 kB
        Bryan Duxbury
      3. thrift-179-v3.patch
        4 kB
        Bryan Duxbury

        Issue Links

          Activity

          Hide
          David Reiss added a comment -

          Already partially implemented in r665256.

          Show
          David Reiss added a comment - Already partially implemented in r665256.
          Hide
          Bryan Duxbury added a comment -

          Forgive me, but where's the partial implementation? I see javadoc for structs, interfaces, and service methods. This issue is supposed to be specifically getting doc strings on the struct fields.

          Show
          Bryan Duxbury added a comment - Forgive me, but where's the partial implementation? I see javadoc for structs, interfaces, and service methods. This issue is supposed to be specifically getting doc strings on the struct fields.
          Hide
          David Reiss added a comment -

          You're right. Sorry for not reading the title carefully enough.

          Show
          David Reiss added a comment - You're right. Sorry for not reading the title carefully enough.
          Hide
          Bryan Duxbury added a comment -

          This patch adds docstrings as described in the issue.

          Show
          Bryan Duxbury added a comment - This patch adds docstrings as described in the issue.
          Hide
          Bryan Duxbury added a comment -

          This is a much better version that factors out some common logic to t_oop_generator.

          Show
          Bryan Duxbury added a comment - This is a much better version that factors out some common logic to t_oop_generator.
          Hide
          David Reiss added a comment -

          Please use braces with all if statements.
          Please don't put "using namespace" in a header file.
          Please use a separate line for each argument in the definition of generate_docstring_comment.
          I think the changes to ThriftTest are unnecessary because of DocTest.thrift.

          Do you think it makes sense to copy the full docstring for each getter and setter?

          Show
          David Reiss added a comment - Please use braces with all if statements. Please don't put "using namespace" in a header file. Please use a separate line for each argument in the definition of generate_docstring_comment. I think the changes to ThriftTest are unnecessary because of DocTest.thrift. Do you think it makes sense to copy the full docstring for each getter and setter?
          Hide
          Bryan Duxbury added a comment -

          This version handles all your comments.

          As far as whether we should add the docstring to both getter and setter, what would the alternative be? Choosing one or the other is pretty arbitrary. There's not really much cost to adding it to both.

          Show
          Bryan Duxbury added a comment - This version handles all your comments. As far as whether we should add the docstring to both getter and setter, what would the alternative be? Choosing one or the other is pretty arbitrary. There's not really much cost to adding it to both.
          Hide
          David Reiss added a comment -

          I made this (semi-)independent change: http://gitweb.thrift-rpc.org/?p=thrift.git;a=commitdiff;h=2e045a6

          I'm planning to commit it on its own.

          This allowed me to make this change to v3: http://gitweb.thrift-rpc.org/?p=thrift.git;a=commitdiff;h=0350c04

          The result is this diff, which I am planning on committing as a separate diff from the first: http://gitweb.thrift-rpc.org/?p=thrift.git;a=treediff;h=0350c04;hp=2e045a6

          Thoughts?

          Show
          David Reiss added a comment - I made this (semi-)independent change: http://gitweb.thrift-rpc.org/?p=thrift.git;a=commitdiff;h=2e045a6 I'm planning to commit it on its own. This allowed me to make this change to v3: http://gitweb.thrift-rpc.org/?p=thrift.git;a=commitdiff;h=0350c04 The result is this diff, which I am planning on committing as a separate diff from the first: http://gitweb.thrift-rpc.org/?p=thrift.git;a=treediff;h=0350c04;hp=2e045a6 Thoughts?
          Hide
          Bryan Duxbury added a comment -

          This looks about right to me. I say commit it.

          Show
          Bryan Duxbury added a comment - This looks about right to me. I say commit it.

            People

            • Assignee:
              Bryan Duxbury
              Reporter:
              Bryan Duxbury
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development