rest.li icon indicating copy to clipboard operation
rest.li copied to clipboard

Pegasus documentation cannot produce valid javadoc with certain symbols

Open jplaisted opened this issue 4 years ago • 7 comments

It seems like Pegasus will do some parsing of documentation tags and spit out the "parsed" strings into the javadoc. This actually makes it invalid.

Example:

  // some enum PDL
  /**
   * Represent the relation greater than, e.g. ownerCount > 5
   */
  GREATER_THAN
    // generated java
    /**
     * Represent the relation greater than, e.g. ownerCount > 5
     * 
     */
    GREATER_THAN,

jplaisted avatar Oct 01 '20 21:10 jplaisted

@jplaisted Hi, Would you mind give me some context? from the examples, are you saying the "> 5" is invalid for you?

BrianPin avatar Oct 07 '20 20:10 BrianPin

In the above I gave the PDL (which used >) and the javadoc it generated (which translated it to >).

> is an invalid character in javadoc.

jplaisted avatar Oct 07 '20 20:10 jplaisted

If you're unaware, Javadoc supports HTML tags. So characters like < and > and invalid on their own and need to use HTML escaping for some things.

I suggest you don't "translate" any documentation here when generating javadoc. Just print it literally. Then PDL annotations can use javadoc syntax directly (so &gt; in PDL becomes &gt; in javadoc, which is valid). The other option is to transform PDL doc to javadoc; so if there is a > in the pegasus doc translate it to &gt;).

jplaisted avatar Oct 07 '20 20:10 jplaisted

got it, I will investigate and get back here

BrianPin avatar Oct 08 '20 07:10 BrianPin

Well, looks like that is the intended design, in restli:

  @Test
  public void testUnescapeDocstring()
  {
    String extracted = PdlParseUtils.extractMarkdown(
        "  /**\n" +
        "   * &lt;div&gt;Some html&lt;/div&gt;\n" +
        "   * &#47;&#42; A comment &#42;&#47;\n" +
        "   */\n");
    assertEquals(extracted,
        "<div>Some html</div>\n" +
        "/* A comment */");
  }

The specific unescape is happening inside

StringEscapeUtils.unescapeHtml4(commentUnescaped);

I think if we change this it will affect a lot.

I would like to discuss with our staffs to gain better decision making.

BrianPin avatar Oct 08 '20 07:10 BrianPin

We agreed that the current behavior isn't ideal, but there are other concerns about how this would affect current use cases. We're tracking this internally and we'll revisit this issue later once we have bandwidth.

evanw555 avatar Nov 04 '20 21:11 evanw555

I created a ticket under my name! FYI

BrianPin avatar Nov 04 '20 22:11 BrianPin