[ZEPPELIN-1387] Support table syntax in markdown interpreter

[1ambda]

What is this PR for?

Support table markdown syntax issued by ZEPPELIN-1387

What type of PR is it?

[Bug Fix | Improvement]

This PR can be categorized as bug fix and improvement since it not only resolves the above issue but also support other markdown syntaxes.

Todos

• - Check the license issue of the pegdown library introduced by this PR

What is the Jira issue?

ZEPPELIN-1387

How should this be tested?

Write markdown texts and compare them with expected html DOMs. I'v also included some tests for this PR.

Screenshots (if appropriate)

### Questions: - Does the licenses files need update? - Does coding style is appropriate? ### Additional Comments

We might solve this issue by implementing custom table plugin for markdown4j by referring the existing work of txtmark.
But I think it is not good idea in regard to coverage, maintainability and efficiency since markdown4j is currently not developed actively and it costs to implement all markdown plugins which is not supported by markdown4j.

[bzz]

@1ambda thank you for improvement! Let me review and get back to you with it.

[AhyoungRyu]

@1ambda Thank you for the contribution! I just shortly look through the screenshot in the description, but comparing to Github markdown support it seems Zeppelin's table doesn't have a table border. If I make the below table in Github,

| Tables   |      Are      |  Cool |
|----------|:-------------:|------:|
| col 1 is |  left-aligned | $1600 |
| col 2 is |    centered   |   $12 |
| col 3 is | right-aligned |    $1 |

it will be shown like this.

But in Zeppelin it doesn't have the border.

[bzz]

The change of library looks great to me, 👍 for having a test case!

Few things to take care of, before merging it:

• for all the code, please make sure it follows project styleguide\code conventions
• as you add a new dependency - it will become a part of the next release convenience binary for Zeppelin and in order to be included, we need to verify it's licence compatibility. Can you please add it to zeppelin-distribution/src/licence_bin/LICENSE
• also, applying some styles for such tables, as noted by @AhyoungRyu sounds like a good improvement as well

[1ambda]

I will refine this PR.

Thanks for the detailed and significant review :) @AhyoungRyu @bzz

[Leemoonsoo]

Thanks @1ambda for the improvement.

Regarding license, 'markdown4j' section in the zeppelin-distribution/src/licence_bin/LICENSE should be removed while the library is no longer included in the binary package.

Also pegdown library does have some transitive dependencies and they also need to be addressed in zeppelin-distribution/src/licence_bin/LICENSE. @1ambda Could you take care of them if they're not already addressed in zeppelin-distribution/src/licence_bin/LICENSE?

[INFO] +- org.pegdown:pegdown:jar:1.6.0:compile
[INFO] |  \- org.parboiled:parboiled-java:jar:1.1.7:compile
[INFO] |     +- org.parboiled:parboiled-core:jar:1.1.7:compile
[INFO] |     +- org.ow2.asm:asm:jar:5.0.3:compile
[INFO] |     +- org.ow2.asm:asm-tree:jar:5.0.3:compile
[INFO] |     +- org.ow2.asm:asm-analysis:jar:5.0.3:compile
[INFO] |     \- org.ow2.asm:asm-util:jar:5.0.3:compile

[1ambda]

@Leemoonsoo Thanks!

I'v added missing transitive deps and removed unused markdown4j dep.

I also found that org.ow2.asm:asm:jar:3.1 is described in zeppelin-distribution/src/licence_bin/LICENSE But already denoted license is different version (3.0.1) compared to pegdown's dependency (5.0.3).

Should I updated org.ow2.asm:asm:jar:3.1 to org.ow2.asm:asm:jar:5.0.3?

[felixcheung]

Markdown4j supports YUML and Websequence, does this support the same?

[1ambda]

@felixcheung Thanks 👍

Pegdown doesn't support YUML, Websequence and Include plugins according to Pegdown Extensions

I think we have 2 options. (or more)

1. Use markdown4j and implement table plugins correctly.
   ** we might need small effort, but we will depend on markdown4j which is not actively developed.
   ** and other markdown syntaxes might be implemented if necessary.
2. Use pegdown and implement YUML, Websequence.

[1ambda]

@AhyoungRyu In regard to table style,

Using CSS (e.g github-markdown-css) would be a better solution rather than adding inline style to every DOM such astable, thead, tbody, tr and td while parsing.

[1ambda]

Now markdown tables are rendered like

[felixcheung]

Personally YUML and Websequence is very handy - I use them quite a bit.
What do other think about this?

[AhyoungRyu]

@1ambda Yeah it's good idea to apply github markdown style to Zeppelin. I tested again and it works well. Definitely better than before!

But I found little bit wired behavior when I made a title.

## This is second level title
### This is third level title

This title code makes a link and it's connected to Zeppelin home. Is it intended?

[1ambda]

@AhyoungRyu

It was due to the ANCHORLINKS option in pegdown. I removed it.

[Leemoonsoo]

Regarding @felixcheung's comment,

I think keeping backward compatibility is always very important.
If i add more possible options after @1ambda 's

• Create new interpreter %pd that uses pegdown and restore %md to use markdown4j
• Add a interpreter property that selects markdown library pegdown/markdown4j.

[AhyoungRyu]

@1ambda Yeah tested again and it looks good👍 Thanks!
(One more thing: Zeppelin's Java block indentation size is 2 spaces :D )

[1ambda]

@Leemoonsoo

I think option 2 (Adding a property) is a better solution. because

1. Adding a new interpreter just for supporting another markdown parser might be confusing to users. They might misunderstand pd interpreter.
2. Pegdown parser will be able to replace markdown4j parser soon if we implement YUML and Websequence. At that time, we can just modify the default parser property of md interpreter instead of requesting for users to change their interpreter type

[1ambda]

I'v restored the markdown4j parser and added markdown.parser.type property to md group.
The pegdown parser will be used only when users specify pegdown value to the property.

Additionally,
It seems that description does't show even though i provide it to markdown/src/main/resources/interpreter-setting.json like

[
  {
    "group": "md",
    "name": "md",
    "className": "org.apache.zeppelin.markdown.MarkdownInterpreter",
    "properties": {
      "markdown.parser.type": {
        "envName": "MARKDOWN_PARSER_TYPE",
        "propertyName": "markdown.parser.type",
        "defaultValue": "markdown4j",
        "description": "Markdown Parser Type. Available values: markdown4j, pegdown. Default = markdown4j"
      }
    }
  }
]

[AhyoungRyu]

@1ambda Great! It works well as expected 👍

If you want to add the interpreter property(markdown.parser.type) to the markdown interpreter, it would be better to provide that information via Zeppelin markdown interpreter docs with configuration table as like the other interpreters do. Nothing much needed. You can just copy and paste the below conf table to markdown.md.

## Configuration
<table class="table-configuration">
  <tr>
    <th>Name</th>
    <th>Default Value</th>
    <th>Description</th>
  </tr>
  <tr>
    <td>markdown.parser.type</td>
    <td>markdown4j </td>
    <td>Markdown Parser Type. <br/> Available values: markdown4j, pegdown.</td>
  </tr>
</table>

Also the below sentences in Overview section need to be updated accordingly.

Markdown is a plain text formatting syntax designed so that it can be converted to HTML. Apache Zeppelin uses markdown4j. For more examples and extension support, please checkout here.

FYI, markdown.md file is under ZEPPELIN_HOME/docs/interpreter/ :)

[bzz]

Is there a reason to catch general Exception instead of more specific sub-classes?

[bzz]

@1ambda 👍 fot tests, looks great to me except for licensing issue raised by Moon and small comment above.

If you could let me know you user name in ASF JIRA I'll be happy to help assigning ZEPPELIN-1387 to you.

[1ambda]

@Leemoonsoo I'v fixed license description. Also, now I understand when to describe licenses in licenses or bin_licenses. Thanks. :)

@bzz I think it is better to use more specific exception types in catch block as you pointed out. Could you review e08929a again?

[Leemoonsoo]

Thanks @1ambda. Looks great to me.
Merge if there're no further discussions.

[AhyoungRyu]

@1ambda Sadly the conflict needs to be resolved first before merging. But only for docs/interpreter/markdown.md this file :)

[1ambda]

@AhyoungRyu

I'v resolved the merge conflict with 8f344db. Could you try again? :)

[1ambda]

CI is failing on only the last job.

[AhyoungRyu]

@1ambda Yeah tested locally and looks good. Thanks for your quick response! Now it's really ready to be merged I think 👍
The other PRs' CI are failing in Selenium test because of the same reason. Currently it's tracked in ZEPPELIN-1449.

[corneadoug]

That CI failure is being handled in #1444, you should be able to rebase and have a green CI soon

[corneadoug]

@1ambda You can rebase now :)

[1ambda]

Now CI is failing on the third build job. :(

I checked out zeppelin/master and merged it on 1ambda/fix-zeppelin-1387.
Should I have used git rebase command instead? I thought using rebase command is not a good idea since it will overwrite
this PR history (comments, etc...)

[1ambda]

Close PR to re-trigger CI

[1ambda]

Open PR to re-trigger CI

[corneadoug]

Usually we prefer rebase since it doesn't pollute the commit history. But its fine, the end result is the same. We sometimes have flaky tests, I will check once the build is finished

[1ambda]

Finally, CI is green! :)

[corneadoug]

Awesome, Merging if there is no more discussions