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

Remove unnecessary text from Javadoc tokens #3796

Closed
rnveach opened this Issue Feb 3, 2017 · 8 comments

Comments

@rnveach
Member

rnveach commented Feb 3, 2017

When looking at some example Javadoc trees, I noticed if the javadoc is pretty big, we almost duplicate the entire contents multiple times in various nodes. We don't see this in the Java tree but this is mostly because of the differences between antlr2 and antlr4. One example of simplicity is CLASS_DEF in java where we just repeat the token name as the text.
I still think there is too much being displayed in Javadoc tree and that we should limit this text by following the Java version and just repeating the token name.

Example:
Javadoc: /** <p>Test</p> Test. */
Tree:

    |  |  |  |  `--JAVADOC ->  <p>Test</p> Test. <EOF> [2:3]
    |  |  |  |      |--TEXT ->  [2:3]
    |  |  |  |      |--HTML_ELEMENT -> <p>Test</p> [2:4]
    |  |  |  |      |  `--PARAGRAPH -> <p>Test</p> [2:4]
    |  |  |  |      |      |--P_TAG_OPEN -> <p> [2:4]
    |  |  |  |      |      |  |--OPEN -> < [2:4]
    |  |  |  |      |      |  |--P_HTML_TAG_NAME -> p [2:5]
    |  |  |  |      |      |  `--CLOSE -> > [2:6]
    |  |  |  |      |      |--TEXT -> Test [2:7]
    |  |  |  |      |      `--P_TAG_CLOSE -> </p> [2:11]
    |  |  |  |      |          |--OPEN -> < [2:11]
    |  |  |  |      |          |--SLASH -> / [2:12]
    |  |  |  |      |          |--P_HTML_TAG_NAME -> p [2:13]
    |  |  |  |      |          `--CLOSE -> > [2:14]
    |  |  |  |      |--TEXT ->  Test.  [2:15]
    |  |  |  |      `--EOF -> <EOF> [2:22]

In this example, I would say the following tokens have unnecessary text: JAVADOC (a complete repeat of the entire javadoc), HTML_ELEMENT, PARAGRAPH, P_TAG_OPEN, P_TAG_CLOSE
We would have to review the all the tokens and find a common rule to how we should apply this, especially for future tokens.

Right now, it might look like that any token that has children shouldn't have text.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 13, 2017

Member

SummaryJavadocCheck is affected by this change as it just gets the text of all the first level nodes, instead of getting the text from the specific children.

Member

rnveach commented Feb 13, 2017

SummaryJavadocCheck is affected by this change as it just gets the text of all the first level nodes, instead of getting the text from the specific children.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 15, 2017

Member

SummaryJavadocCheck should be updated to work on new tree.

Member

romani commented Feb 15, 2017

SummaryJavadocCheck should be updated to work on new tree.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 28, 2017

Member

Any documentation that prints the Javadoc tree is also affected by this change.
Example: http://checkstyle.sourceforge.net/writingjavadocchecks.html#Tool_to_print_Javadoc_tree_structure

Member

rnveach commented Feb 28, 2017

Any documentation that prints the Javadoc tree is also affected by this change.
Example: http://checkstyle.sourceforge.net/writingjavadocchecks.html#Tool_to_print_Javadoc_tree_structure

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jun 15, 2017

Collaborator

would these be acceptable:

/** <p>Test</p> Test. */

JAVADOC ->  [0:0]
|--TEXT -> /**  [0:0]
|--HTML_ELEMENT ->  [0:4]
|   `--PARAGRAPH ->  [0:4]
|       |--P_TAG_OPEN ->  [0:4]
|       |   |--OPEN -> < [0:4]
|       |   |--P_HTML_TAG_NAME -> p [0:5]
|       |   `--CLOSE -> > [0:6]
|       |--TEXT -> Test [0:7]
|       `--P_TAG_CLOSE ->  [0:11]
|           |--OPEN -> < [0:11]
|           |--SLASH -> / [0:12]
|           |--P_HTML_TAG_NAME -> p [0:13]
|           `--CLOSE -> > [0:14]
|--TEXT ->  Test. */ [0:15]
`--EOF -> <EOF> [0:24]

<p>\nMy {@code Class}\n</p>\n@see never<EOF>

JAVADOC ->  [0:0]
|--HTML_ELEMENT ->  [0:0]
|   `--PARAGRAPH ->  [0:0]
|       |--P_TAG_OPEN ->  [0:0]
|       |   |--OPEN -> < [0:0]
|       |   |--P_HTML_TAG_NAME -> p [0:1]
|       |   `--CLOSE -> > [0:2]
|       |--NEWLINE -> \n [0:3]
|       |--TEXT -> My  [1:0]
|       |--JAVADOC_INLINE_TAG ->  [1:3]
|       |   |--JAVADOC_INLINE_TAG_START -> { [1:3]
|       |   |--CODE_LITERAL -> @code [1:4]
|       |   |--WS ->   [1:9]
|       |   |--TEXT -> Class [1:10]
|       |   `--JAVADOC_INLINE_TAG_END -> } [1:16]
|       |--NEWLINE -> \n [1:17]
|       `--P_TAG_CLOSE ->  [2:0]
|           |--OPEN -> < [2:0]
|           |--SLASH -> / [2:1]
|           |--P_HTML_TAG_NAME -> p [2:2]
|           `--CLOSE -> > [2:3]
|--NEWLINE -> \n [2:4]
|--JAVADOC_TAG ->  [3:0]
|   |--SEE_LITERAL -> @see [3:0]
|   |--WS ->   [3:4]
|   `--REFERENCE ->  [3:5]
|       `--PACKAGE -> never [3:5]
`--EOF -> <EOF> [3:10]
Collaborator

ps-sp commented Jun 15, 2017

would these be acceptable:

/** <p>Test</p> Test. */

JAVADOC ->  [0:0]
|--TEXT -> /**  [0:0]
|--HTML_ELEMENT ->  [0:4]
|   `--PARAGRAPH ->  [0:4]
|       |--P_TAG_OPEN ->  [0:4]
|       |   |--OPEN -> < [0:4]
|       |   |--P_HTML_TAG_NAME -> p [0:5]
|       |   `--CLOSE -> > [0:6]
|       |--TEXT -> Test [0:7]
|       `--P_TAG_CLOSE ->  [0:11]
|           |--OPEN -> < [0:11]
|           |--SLASH -> / [0:12]
|           |--P_HTML_TAG_NAME -> p [0:13]
|           `--CLOSE -> > [0:14]
|--TEXT ->  Test. */ [0:15]
`--EOF -> <EOF> [0:24]

<p>\nMy {@code Class}\n</p>\n@see never<EOF>

JAVADOC ->  [0:0]
|--HTML_ELEMENT ->  [0:0]
|   `--PARAGRAPH ->  [0:0]
|       |--P_TAG_OPEN ->  [0:0]
|       |   |--OPEN -> < [0:0]
|       |   |--P_HTML_TAG_NAME -> p [0:1]
|       |   `--CLOSE -> > [0:2]
|       |--NEWLINE -> \n [0:3]
|       |--TEXT -> My  [1:0]
|       |--JAVADOC_INLINE_TAG ->  [1:3]
|       |   |--JAVADOC_INLINE_TAG_START -> { [1:3]
|       |   |--CODE_LITERAL -> @code [1:4]
|       |   |--WS ->   [1:9]
|       |   |--TEXT -> Class [1:10]
|       |   `--JAVADOC_INLINE_TAG_END -> } [1:16]
|       |--NEWLINE -> \n [1:17]
|       `--P_TAG_CLOSE ->  [2:0]
|           |--OPEN -> < [2:0]
|           |--SLASH -> / [2:1]
|           |--P_HTML_TAG_NAME -> p [2:2]
|           `--CLOSE -> > [2:3]
|--NEWLINE -> \n [2:4]
|--JAVADOC_TAG ->  [3:0]
|   |--SEE_LITERAL -> @see [3:0]
|   |--WS ->   [3:4]
|   `--REFERENCE ->  [3:5]
|       `--PACKAGE -> never [3:5]
`--EOF -> <EOF> [3:10]
@Vladlis

This comment has been minimized.

Show comment
Hide comment
@Vladlis

Vladlis Jun 15, 2017

Member

@ps-sp
I think JAVADOC should also have the whole text.

Member

Vladlis commented Jun 15, 2017

@ps-sp
I think JAVADOC should also have the whole text.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 15, 2017

Member

would these be acceptable:
|--HTML_ELEMENT -> [0:4]

If we are displaying no text (not talking about full whitespace text) for a node, it should just repeat the node name like the Java grammar does.

any token that has children shouldn't have text.

This is the rule I feel we should follow.

I think JAVADOC should also have the whole text.

I don't. We don't do anything like that in Java grammar where we repeat the entire text of the Java file.
This is one of the main points of this issue.

Member

rnveach commented Jun 15, 2017

would these be acceptable:
|--HTML_ELEMENT -> [0:4]

If we are displaying no text (not talking about full whitespace text) for a node, it should just repeat the node name like the Java grammar does.

any token that has children shouldn't have text.

This is the rule I feel we should follow.

I think JAVADOC should also have the whole text.

I don't. We don't do anything like that in Java grammar where we repeat the entire text of the Java file.
This is one of the main points of this issue.

@Vladlis

This comment has been minimized.

Show comment
Hide comment
@Vladlis

Vladlis Jun 15, 2017

Member

@rnveach ok
@ps-sp lets make it similar to how java AST is printed as @rnveach points out

Member

Vladlis commented Jun 15, 2017

@rnveach ok
@ps-sp lets make it similar to how java AST is printed as @rnveach points out

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 21, 2017

Member

fix is merged.

Member

romani commented Jun 21, 2017

fix is merged.

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