groovy
  1. groovy
  2. GROOVY-4136

gapi document generation ignores access modifiers set on groovydoc

    Details

    • Type: Improvement Improvement
    • Status: Open Open
    • Priority: Trivial Trivial
    • Resolution: Unresolved
    • Affects Version/s: 1.8-beta-1
    • Fix Version/s: None
    • Component/s: GroovyDoc
    • Labels:
      None
    • Number of attachments :
      0

      Description

      org.codehaus.groovy.ant.Groovydoc supports attributes like 'private'/'protected'/'public' but it seems like they are ignored in the gapi documentation generation. The documentation generated contains package-private details too, which it should not.

      For more related information, see GROOVY-4135.

        Issue Links

          Activity

          Hide
          Paul King added a comment -

          Yes, package and protected scopes are supported as options but currently ignored.

          Show
          Paul King added a comment - Yes, package and protected scopes are supported as options but currently ignored.
          Hide
          Roshan Dawrani added a comment -

          org.codehaus.groovy.ant.Groovydoc has a little weird way of getting the access modifier set - through private / protected / public / package properties.

          It's weird because if someone sets private = true and public = false, it will be meaningless - unless if you change other properties when one changes.

          Ant Javadoc task on the other hand provides something cleaner through just one property called "access" that can be set to protected/public/private/package - so no chance of conflicting access modifier from being set.

          Not sure how freely we can change the weird but public API of org.codehaus.groovy.ant.Groovydoc!

          Show
          Roshan Dawrani added a comment - org.codehaus.groovy.ant.Groovydoc has a little weird way of getting the access modifier set - through private / protected / public / package properties. It's weird because if someone sets private = true and public = false, it will be meaningless - unless if you change other properties when one changes. Ant Javadoc task on the other hand provides something cleaner through just one property called "access" that can be set to protected/public/private/package - so no chance of conflicting access modifier from being set. Not sure how freely we can change the weird but public API of org.codehaus.groovy.ant.Groovydoc!
          Hide
          Paul King added a comment -

          Looks like Ant Javadoc supports 'Private/Public/Protected/Package' as well as 'access'. I am not sure about the case sensitivity of those attributes. I thought Ant was case insensitive in which case we have the former. I think it is worth changing to support both. We probably don't do any error checking at the moment. Again we should align with whatever the Java implementation does.

          Show
          Paul King added a comment - Looks like Ant Javadoc supports 'Private/Public/Protected/Package' as well as 'access'. I am not sure about the case sensitivity of those attributes. I thought Ant was case insensitive in which case we have the former. I think it is worth changing to support both. We probably don't do any error checking at the moment. Again we should align with whatever the Java implementation does.
          Hide
          Roshan Dawrani added a comment -

          Yes, Ant Javadoc seems to be supporting 'Private/Public/Protected/Package' as well as 'access'. All it does is that it passes all the options, which are set to true, to the javadoc command, which then handles them.

          It seems javadoc rejects if more than one modifier is passed, as:

          javadoc: error - More than one of -public, -private, -package, or -protected specified.
          

          That makes life simpler. We can do the same.

          Show
          Roshan Dawrani added a comment - Yes, Ant Javadoc seems to be supporting 'Private/Public/Protected/Package' as well as 'access'. All it does is that it passes all the options, which are set to true, to the javadoc command, which then handles them. It seems javadoc rejects if more than one modifier is passed, as: javadoc: error - More than one of -public, -private, -package, or -protected specified. That makes life simpler. We can do the same.
          Hide
          Roshan Dawrani added a comment -

          Handling of modifiers in groovydoc seems a little messed up.

          Paul, any idea why Groovifier removes "public" modifier in its visitDefault() when it converts java AST to groovy AST?

          It becomes a problem later on in modifiers handling during the documentation generation (in SimpleGroovyClassDocAssembler#processModifiers()) because you don't know whether "public" was never specified or cleared. It goes by AST node's type here, which Groovifier messes up when it clears "public".

          Show
          Roshan Dawrani added a comment - Handling of modifiers in groovydoc seems a little messed up. Paul, any idea why Groovifier removes "public" modifier in its visitDefault() when it converts java AST to groovy AST? It becomes a problem later on in modifiers handling during the documentation generation (in SimpleGroovyClassDocAssembler#processModifiers()) because you don't know whether "public" was never specified or cleared. It goes by AST node's type here, which Groovifier messes up when it clears "public".
          Hide
          Paul King added a comment -

          Yes, that is mostly historical. The Groovifier is used both for GroovyDoc's purposes and for the Java2Groovy conversion program.
          For the latter case, when converting a Java program into Groovy you normally want to get rid of 'public' for methods and classes as that is the default for Groovy and is only added noise. However, when treating Groovyified Java programs for GroovyDoc's purposes it is unwanted behavior. There now is an option to turn off that behavior.

          It still needs lots more tidying up and refactoring but it mostly supports the access scopes now. The visibility checking needs to be pushed into the GroovyDoc classes not spread all over the templates. We also need to create an extra boolean in SimpleGroovyClassDoc to remember whether it is representing Java or Groovy. That would allow modifier visibility and representation to be fixed a little better still from what we have right now.

          Show
          Paul King added a comment - Yes, that is mostly historical. The Groovifier is used both for GroovyDoc's purposes and for the Java2Groovy conversion program. For the latter case, when converting a Java program into Groovy you normally want to get rid of 'public' for methods and classes as that is the default for Groovy and is only added noise. However, when treating Groovyified Java programs for GroovyDoc's purposes it is unwanted behavior. There now is an option to turn off that behavior. It still needs lots more tidying up and refactoring but it mostly supports the access scopes now. The visibility checking needs to be pushed into the GroovyDoc classes not spread all over the templates. We also need to create an extra boolean in SimpleGroovyClassDoc to remember whether it is representing Java or Groovy. That would allow modifier visibility and representation to be fixed a little better still from what we have right now.
          Hide
          Roshan Dawrani added a comment - - edited

          Thanks for getting back, Paul. That will help me move forward. I may have to come back again in case of doubts, because, unlike in regular code, documentation generation regression issues, if introduced, will not come out so easily.

          So, I will introduce a flag on GroovyVerifier so that it knows whether it is coming from Java2Groovy or from groovydoc - so that it can do noise reduction for Java2Groovy and not come in the way for groovydoc.

          I think isGroovy flag is generally in place - I will pass it on to SimpleGroovyClassDoc and related classes. That will surely be required when dealing with modifiers because absence of any modifier means package-private for Java classes and public for groovy classes.

          Show
          Roshan Dawrani added a comment - - edited Thanks for getting back, Paul. That will help me move forward. I may have to come back again in case of doubts, because, unlike in regular code, documentation generation regression issues, if introduced, will not come out so easily. So, I will introduce a flag on GroovyVerifier so that it knows whether it is coming from Java2Groovy or from groovydoc - so that it can do noise reduction for Java2Groovy and not come in the way for groovydoc. I think isGroovy flag is generally in place - I will pass it on to SimpleGroovyClassDoc and related classes. That will surely be required when dealing with modifiers because absence of any modifier means package-private for Java classes and public for groovy classes.
          Hide
          Paul King added a comment - - edited

          You might want to check my recent commit. It has some of this in place already. The isGroovy change you are suggesting to SimpleGroovyClassDoc is the next logical thing to look at. I probably won't get time again for a few days and will be very happy for someone else to help with the necessary refactoring.

          Show
          Paul King added a comment - - edited You might want to check my recent commit. It has some of this in place already. The isGroovy change you are suggesting to SimpleGroovyClassDoc is the next logical thing to look at. I probably won't get time again for a few days and will be very happy for someone else to help with the necessary refactoring.
          Hide
          Roshan Dawrani added a comment -

          Oh, you have made some changes already! I didn't get that earlier. I will look at something else then

          I wanted to check one more thing. The grammar merge that Guillaume is working on - that can also have impact on the groovydoc generation, right? If yes, I am not sure whether that part is being tested in the grammar merging.

          Show
          Roshan Dawrani added a comment - Oh, you have made some changes already! I didn't get that earlier. I will look at something else then I wanted to check one more thing. The grammar merge that Guillaume is working on - that can also have impact on the groovydoc generation, right? If yes, I am not sure whether that part is being tested in the grammar merging.
          Hide
          Roshan Dawrani added a comment -

          Sure. I will try taking the scope related changes forward.

          Show
          Roshan Dawrani added a comment - Sure. I will try taking the scope related changes forward.
          Hide
          Guillaume Laforge added a comment -

          The grammar changes will require quite some work so we can integrate that in Groovy, so it's not for today.

          Show
          Guillaume Laforge added a comment - The grammar changes will require quite some work so we can integrate that in Groovy, so it's not for today.
          Hide
          Paul King added a comment -

          Yes, minimal error handling of the strange cases is in place, e.g. multiple settings. Plus the access scope is mostly respected - so the recently discussed classes like NodeChildren will be gone.

          There are some things which perhaps need further discussion. For Groovy classes I suspect people will be expecting 'public' to be missing. At the moment we still leave public on at the top most class level (perhaps we should turn that off - e.g. look at CliBuilder) but off for e.g. methods. Currently Java is done the same way but perhaps people expect the methods to be public as per normal Java conventions (e.g. look at a method in Expando). The isGroovy flag will help with this (but we also need a reference between members and the defining class - there is an incomplete hook for that already).

          Just on overall design, I think there are two approaches we can take as we evolve Groovydoc further. One path we can take is to make the Java follow Groovy conventions. After all, why should I know or even care about what the base language is and won't getting rid of some of the noise assist making the Java easier to read? Perhaps, but it gives us no way to distinguish Java specific things like package private.

          So, the other approach is to treat the two languages differently, which is mostly what we do now. For this to really work better, I think we need some more visual clues so that I can tell whether I am in Groovy and have something public or in Java and have package private. At the moment, it would be hard to tell. So, I think we need to work on that too. Perhaps some refinement of the CSS colors or perhaps a Java vs Groovy icon or some other indication on the page itself.

          Show
          Paul King added a comment - Yes, minimal error handling of the strange cases is in place, e.g. multiple settings. Plus the access scope is mostly respected - so the recently discussed classes like NodeChildren will be gone. There are some things which perhaps need further discussion. For Groovy classes I suspect people will be expecting 'public' to be missing. At the moment we still leave public on at the top most class level (perhaps we should turn that off - e.g. look at CliBuilder) but off for e.g. methods. Currently Java is done the same way but perhaps people expect the methods to be public as per normal Java conventions (e.g. look at a method in Expando). The isGroovy flag will help with this (but we also need a reference between members and the defining class - there is an incomplete hook for that already). Just on overall design, I think there are two approaches we can take as we evolve Groovydoc further. One path we can take is to make the Java follow Groovy conventions. After all, why should I know or even care about what the base language is and won't getting rid of some of the noise assist making the Java easier to read? Perhaps, but it gives us no way to distinguish Java specific things like package private. So, the other approach is to treat the two languages differently, which is mostly what we do now. For this to really work better, I think we need some more visual clues so that I can tell whether I am in Groovy and have something public or in Java and have package private. At the moment, it would be hard to tell. So, I think we need to work on that too. Perhaps some refinement of the CSS colors or perhaps a Java vs Groovy icon or some other indication on the page itself.
          Hide
          Paul King added a comment -

          Also, on the grammar changes. They will mostly improve the ability to process the content inside comments. E.g. extracting the @author tags easily rather than regex scrapping the information and keeping such info in its own data structure - helpful for smart @link's etc. The other processing, e.g. modifiers, method processing etc. won't be affected so much. There was also discussion about a doclet approach but that is also a long way off at the moment.

          Show
          Paul King added a comment - Also, on the grammar changes. They will mostly improve the ability to process the content inside comments. E.g. extracting the @author tags easily rather than regex scrapping the information and keeping such info in its own data structure - helpful for smart @link's etc. The other processing, e.g. modifiers, method processing etc. won't be affected so much. There was also discussion about a doclet approach but that is also a long way off at the moment.
          Hide
          Paul King added a comment -

          The isGroovy flag is now in SimpleGroovyClassDoc and there is a visibility indicator [Groovy] or [Java] before each class declaration in the GroovyDoc. This will enable us to start correcting want the temples do, e.g. no 'public' in front of a normal Groovy class but retain for a Java class. And so forth for methods etc.

          Show
          Paul King added a comment - The isGroovy flag is now in SimpleGroovyClassDoc and there is a visibility indicator [Groovy] or [Java] before each class declaration in the GroovyDoc. This will enable us to start correcting want the temples do, e.g. no 'public' in front of a normal Groovy class but retain for a Java class. And so forth for methods etc.

            People

            • Assignee:
              Unassigned
              Reporter:
              Roshan Dawrani
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated: