Thrift
  1. Thrift
  2. THRIFT-681

The HTML generator does not handle JavaDoc style comments very well

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.2
    • Fix Version/s: 0.9
    • Component/s: Compiler (General)
    • Labels:
      None

      Description

      If you create comments using the standard JavaDoc conventions of @param and @exception, the output that gets generated isn't the cleanest. It would be better if the list of parameters and exceptions were placed in a table with the appropriate headers rather than just outputting the @param and @exception tags into the HTML output.

      1. thrift-682-v2.patch
        1 kB
        Bryan Duxbury
      2. thrift-681-inline-parameter-doc-generation.png
        24 kB
        Kevin Burnett
      3. thrift-681-inline-parameter-doc-generation.patch
        24 kB
        Kevin Burnett
      4. t_html_generator_JavaDoc.v2.patch
        8 kB
        Adrian Muraru
      5. t_html_generator_JavaDoc.patch
        7 kB
        John Bartak
      6. screenshot-1.jpg
        84 kB
        John Bartak
      7. Screenshot.png
        30 kB
        Adrian Muraru

        Activity

        Hide
        Roger Meier added a comment -

        just committed the fix for THRIFT-1588 ... the 2nd issue we had
        ;-r

        Show
        Roger Meier added a comment - just committed the fix for THRIFT-1588 ... the 2nd issue we had ;-r
        Hide
        Kevin Burnett added a comment -

        Note that it looks like this change broke the build (since the header file on the patch was originally missed, and since adding the header file didn't fix the build), but I think it was actually a change on another ticket, possibly http://svn.apache.org/viewvc?view=revision&revision=1333239 (see https://builds.apache.org/job/Thrift/449/).

        Show
        Kevin Burnett added a comment - Note that it looks like this change broke the build (since the header file on the patch was originally missed, and since adding the header file didn't fix the build), but I think it was actually a change on another ticket, possibly http://svn.apache.org/viewvc?view=revision&revision=1333239 (see https://builds.apache.org/job/Thrift/449/ ).
        Hide
        Hudson added a comment -

        Integrated in Thrift #450 (See https://builds.apache.org/job/Thrift/450/)
        THRIFT-681 The HTML generator does not handle JavaDoc style comments very well
        add missing file t_html_generator.h (Revision 1333398)

        Result = FAILURE
        roger : http://svn.apache.org/viewvc/?view=rev&rev=1333398
        Files :

        • /thrift/trunk/compiler/cpp/src/generate/t_html_generator.h
        Show
        Hudson added a comment - Integrated in Thrift #450 (See https://builds.apache.org/job/Thrift/450/ ) THRIFT-681 The HTML generator does not handle JavaDoc style comments very well add missing file t_html_generator.h (Revision 1333398) Result = FAILURE roger : http://svn.apache.org/viewvc/?view=rev&rev=1333398 Files : /thrift/trunk/compiler/cpp/src/generate/t_html_generator.h
        Hide
        Roger Meier added a comment -

        just committed the missing file
        ;-r

        Show
        Roger Meier added a comment - just committed the missing file ;-r
        Hide
        Nathaniel Cook added a comment -

        Looks like t_html_generator.h didn't get patched in. I was able to do it manually with the thrift-681-inline patch, but otherwise it won't compile.

        Show
        Nathaniel Cook added a comment - Looks like t_html_generator.h didn't get patched in. I was able to do it manually with the thrift-681-inline patch, but otherwise it won't compile.
        Hide
        Hudson added a comment -

        Integrated in Thrift #448 (See https://builds.apache.org/job/Thrift/448/)
        THRIFT-681. The HTML generator does not handle JavaDoc style comments very well

        Patch: Kevin Burnett (Revision 1333222)

        Result = FAILURE
        bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1333222
        Files :

        • /thrift/trunk/compiler/cpp/src/generate/t_html_generator.cc
        • /thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc
        Show
        Hudson added a comment - Integrated in Thrift #448 (See https://builds.apache.org/job/Thrift/448/ ) THRIFT-681 . The HTML generator does not handle JavaDoc style comments very well Patch: Kevin Burnett (Revision 1333222) Result = FAILURE bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1333222 Files : /thrift/trunk/compiler/cpp/src/generate/t_html_generator.cc /thrift/trunk/compiler/cpp/src/generate/t_java_generator.cc
        Hide
        Bryan Duxbury added a comment -

        Committed. Thanks for the patch, Kevin!

        Show
        Bryan Duxbury added a comment - Committed. Thanks for the patch, Kevin!
        Hide
        Kevin Burnett added a comment -

        Attached please find a patch that applies cleanly to http://svn.apache.org/repos/asf/thrift/trunk@1332766 and implements the "comments on individual elements" style of html generation. The screenshot demonstrates the parameter documentation created by thrift -gen html test/DocTest.thrift. Exceptions with documentation also appear in table format in the generated html documentation. Note that a minified version of bootstrap.css (apache licensed) has been bundled as well in order to spruce up the css styles. I worked with a colleague, Kevin Radloff (radsaq), on these changes. Thanks for thrift!

        Show
        Kevin Burnett added a comment - Attached please find a patch that applies cleanly to http://svn.apache.org/repos/asf/thrift/trunk@1332766 and implements the "comments on individual elements" style of html generation. The screenshot demonstrates the parameter documentation created by thrift -gen html test/DocTest.thrift. Exceptions with documentation also appear in table format in the generated html documentation. Note that a minified version of bootstrap.css (apache licensed) has been bundled as well in order to spruce up the css styles. I worked with a colleague, Kevin Radloff (radsaq), on these changes. Thanks for thrift!
        Hide
        Bryan Duxbury added a comment -

        I'd commit a patch that applied cleanly and implemented "comments on individual elements" style.

        Show
        Bryan Duxbury added a comment - I'd commit a patch that applied cleanly and implemented "comments on individual elements" style.
        Hide
        Adrian Muraru added a comment -

        Hey guys, any chance to see these changes in a future version of thrift ?
        I'm willing to port the patch to current HEAD.

        best,
        -adrian

        Show
        Adrian Muraru added a comment - Hey guys, any chance to see these changes in a future version of thrift ? I'm willing to port the patch to current HEAD. best, -adrian
        Hide
        Adrian Muraru added a comment -

        One more vote for inline doc style, it tends to be easily maintainable that way.
        Bryan, I checked your patch but doesn't seem to be the intended one - guess you uploaded a different patch.

        Show
        Adrian Muraru added a comment - One more vote for inline doc style, it tends to be easily maintainable that way. Bryan, I checked your patch but doesn't seem to be the intended one - guess you uploaded a different patch.
        Hide
        David Reiss added a comment -

        My preference is for the latter. I disagree with John's last comment. Specifically, I think it is easier to have the docs exist once right next to the parameter they refer to, rather than have the docs all clustered at the top with the names repeated (or out-of-sync) and no immediate indication of the type.

        Kind of like having a docblock above each method, rather than just having them all at the top of the class.

        Another way I think of it is that the arguments are like members of a structure that are being sent along with the RPC, and the structure members are documented individually.

        That said, I don't think it would destroy the Thrift project if we went the other way.

        Show
        David Reiss added a comment - My preference is for the latter. I disagree with John's last comment. Specifically, I think it is easier to have the docs exist once right next to the parameter they refer to, rather than have the docs all clustered at the top with the names repeated (or out-of-sync) and no immediate indication of the type. Kind of like having a docblock above each method, rather than just having them all at the top of the class. Another way I think of it is that the arguments are like members of a structure that are being sent along with the RPC, and the structure members are documented individually. That said, I don't think it would destroy the Thrift project if we went the other way.
        Hide
        Bryan Duxbury added a comment -

        I just spent a few minutes fixing the spacing and reviewing the v2 patch to come up with this cleaner version. But then I realized that we hadn't come to a clear conclusion as to whether we wanted to encourage the "@param in above block" or "comments on individual elements" doc style.

        Show
        Bryan Duxbury added a comment - I just spent a few minutes fixing the spacing and reviewing the v2 patch to come up with this cleaner version. But then I realized that we hadn't come to a clear conclusion as to whether we wanted to encourage the "@param in above block" or "comments on individual elements" doc style.
        Hide
        Adrian Muraru added a comment -

        I enhanced John's patch to read the parameters doc from inner docblocks, please review it.

        Show
        Adrian Muraru added a comment - I enhanced John's patch to read the parameters doc from inner docblocks, please review it.
        Hide
        John Bartak added a comment -

        I'm also concerned about the readability of the documentation in the .thrift file. I think it is easier to read the JavaDoc style comments than putting each parameter on a separate line with a comment above it.

        Show
        John Bartak added a comment - I'm also concerned about the readability of the documentation in the .thrift file. I think it is easier to read the JavaDoc style comments than putting each parameter on a separate line with a comment above it.
        Hide
        Lars Francke added a comment -

        About the duplicated parameters: Have a look at the file David pointed out to you. It shows how function parameters ought to be documented.

        I'm using the comment style from the DocTest.thrift but there is no documentation generated for function parameters either:

          /**
           * Disables a table (takes it off-line) If it is being served, the master
           * will tell the servers to stop serving it.
           */
          void disableTable(
            /** name of the table */
            1:Bytes tableName
          ) throws (1:IOError io)
        

        It would be nice if this format would be supported by the HTML compiler. The output your patch produces looks very nice. I think it would be good if that patch was adapted so it uses this "official" documentation style. It would be even nicer if this style was documented somewhere outside of the test. I'll try to find a place in the wiki tomorrow where this fits.

        Show
        Lars Francke added a comment - About the duplicated parameters: Have a look at the file David pointed out to you. It shows how function parameters ought to be documented. I'm using the comment style from the DocTest.thrift but there is no documentation generated for function parameters either: /** * Disables a table (takes it off-line) If it is being served, the master * will tell the servers to stop serving it. */ void disableTable( /** name of the table */ 1:Bytes tableName ) throws (1:IOError io) It would be nice if this format would be supported by the HTML compiler. The output your patch produces looks very nice. I think it would be good if that patch was adapted so it uses this "official" documentation style. It would be even nicer if this style was documented somewhere outside of the test. I'll try to find a place in the wiki tomorrow where this fits.
        Hide
        David Reiss added a comment -

        Yeah, I'm pretty sure what is going on here is that the compiler is just byte-copying your javadoc-style comment from the thrift file to the java file. Try using docblock comments on the parameters themselves (see that file I pointed to).

        Show
        David Reiss added a comment - Yeah, I'm pretty sure what is going on here is that the compiler is just byte-copying your javadoc-style comment from the thrift file to the java file. Try using docblock comments on the parameters themselves (see that file I pointed to).
        Hide
        John Bartak added a comment - - edited

        The section from the .thrift file that generated the attached screenshot:

        	/** 
        	* Changes a message's status
        	* @param personId the person changing the status
        	* @param messageId the message to change the status of
        	* @param newStatus the new status of the message
        	* @exception AccessDeniedException thrown if the person does not have access to the message
        	*/
        	void UpdateMessageStatus(1:string personId,2:string messageId,3:MessageData.Status newStatus) throws 
        (1:Shared.SocialException socialError,2:Shared.SocialDatabaseException databaseError,3:Shared.BadRevisionException 
        revisionException,4:Shared.ResourceNotFoundException notFoundException,5:Shared.AccessDeniedException accessError),
        	
        

        The Java compiler generates the following code from the above definition:

            /**
             * Changes a message's status
             * @param personId the person changing the status
             * @param messageId the message to change the status of
             * @param newStatus the new status of the message
             * @exception AccessDeniedException thrown if the person does not have access to the message
             * 
             * @param personId
             * @param messageId
             * @param newStatus
             */
            public void UpdateMessageStatus(String personId, String messageId, com.autodesk.social.serviceContracts.Message.Status newStatus) 
        throws com.autodesk.social.serviceContracts.SocialException, com.autodesk.social.serviceContracts.SocialDatabaseException, 
        com.autodesk.social.serviceContracts.BadRevisionException, com.autodesk.social.serviceContracts.ResourceNotFoundException, 
        com.autodesk.social.serviceContracts.AccessDeniedException, TException;
        

        This isn't totally ideal since the parameters get duplicated – but that seems like more of an issue with the Java compiler.

        Show
        John Bartak added a comment - - edited The section from the .thrift file that generated the attached screenshot: /** * Changes a message's status * @param personId the person changing the status * @param messageId the message to change the status of * @param newStatus the new status of the message * @exception AccessDeniedException thrown if the person does not have access to the message */ void UpdateMessageStatus(1:string personId,2:string messageId,3:MessageData.Status newStatus) throws (1:Shared.SocialException socialError,2:Shared.SocialDatabaseException databaseError,3:Shared.BadRevisionException revisionException,4:Shared.ResourceNotFoundException notFoundException,5:Shared.AccessDeniedException accessError), The Java compiler generates the following code from the above definition: /** * Changes a message's status * @param personId the person changing the status * @param messageId the message to change the status of * @param newStatus the new status of the message * @exception AccessDeniedException thrown if the person does not have access to the message * * @param personId * @param messageId * @param newStatus */ public void UpdateMessageStatus( String personId, String messageId, com.autodesk.social.serviceContracts.Message.Status newStatus) throws com.autodesk.social.serviceContracts.SocialException, com.autodesk.social.serviceContracts.SocialDatabaseException, com.autodesk.social.serviceContracts.BadRevisionException, com.autodesk.social.serviceContracts.ResourceNotFoundException, com.autodesk.social.serviceContracts.AccessDeniedException, TException; This isn't totally ideal since the parameters get duplicated – but that seems like more of an issue with the Java compiler.
        Hide
        David Reiss added a comment -

        Can you provide an example .thrift file to demonstrate what you mean by "The Java compiler inserts the comments from the .thrift file into the generated .java file."?

        Show
        David Reiss added a comment - Can you provide an example .thrift file to demonstrate what you mean by "The Java compiler inserts the comments from the .thrift file into the generated .java file."?
        Hide
        John Bartak added a comment -

        Atttached screenshot showing result of using this patch

        Show
        John Bartak added a comment - Atttached screenshot showing result of using this patch
        Hide
        John Bartak added a comment -

        The Java compiler inserts the comments from the .thrift file into the generated .java file. So it would be nice if the HTML compiler respected the JavaDoc style comments.

        I will be attaching a patch that actually does this.

        Show
        John Bartak added a comment - The Java compiler inserts the comments from the .thrift file into the generated .java file. So it would be nice if the HTML compiler respected the JavaDoc style comments. I will be attaching a patch that actually does this.
        Hide
        David Reiss added a comment -

        See test/DocTest.thrift for the proper way of documenting parameters. We don't have an example for exceptions, but they work the same way.

        Show
        David Reiss added a comment - See test/DocTest.thrift for the proper way of documenting parameters. We don't have an example for exceptions, but they work the same way.

          People

          • Assignee:
            Kevin Burnett
            Reporter:
            John Bartak
          • Votes:
            2 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development