Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.3
    • Component/s: Module - Markdown, Modules
    • Labels:
      None
    • Flags:
      Patch

      Description

      Markdown is a widespread Markup language. It would be nice if there was a Doxia module for Markdown.

      Here is a proposed simple implementation that defers all the parsing and rendering to PegDown (Apache License V2), which is the most reliable Java library for Markdown.

      1. doxia-module-markdown.patch
        18 kB
        Julien Nicoulaud
      2. doxia-module-markdown-2.patch
        16 kB
        Julien Nicoulaud
      3. doxia-module-markdown-3.patch
        108 kB
        Julien Nicoulaud
      4. doxia-module-markdown-4.patch
        0.5 kB
        Julien Nicoulaud

        Issue Links

          Activity

          Hide
          Lukas Theussl added a comment -

          Thanks! I have added the module to the sandbox for the moment.

          Some questions/comments:

          • since MarkdownSink is not implemented it would be better to remove the class altogether, or do you plan to provide an implementation?
          • the MarkdownParser emits the parsing result as rawText, this means the parser is only usable for html output (see related DOXIA-183). This should be re-written to emit proper doxia events (e.g. by chaining a XhtmlParser).
          • I'd prefer some more realistic test cases before promoting this out of the sandbox
          Show
          Lukas Theussl added a comment - Thanks! I have added the module to the sandbox for the moment. Some questions/comments: since MarkdownSink is not implemented it would be better to remove the class altogether, or do you plan to provide an implementation? the MarkdownParser emits the parsing result as rawText, this means the parser is only usable for html output (see related DOXIA-183 ). This should be re-written to emit proper doxia events (e.g. by chaining a XhtmlParser). I'd prefer some more realistic test cases before promoting this out of the sandbox
          Hide
          Julien Nicoulaud added a comment -

          Thanks for reviewing the patch !

          Here is a new one (or am I supposed to commit in the sandbox ?):

          • Removed MarkdownSink
          • MarkdownParser now extends XhtmlParser (which means doxia-module-markdown depends on doxia-module-xhtml and should be the last module in doxia-modules)
          • Added some test cases to make sure doxia events are fired. Not all events types are tested, tell me if you want more.
          • Upgraded PegDown library to last release
          Show
          Julien Nicoulaud added a comment - Thanks for reviewing the patch ! Here is a new one (or am I supposed to commit in the sandbox ?): Removed MarkdownSink MarkdownParser now extends XhtmlParser (which means doxia-module-markdown depends on doxia-module-xhtml and should be the last module in doxia-modules) Added some test cases to make sure doxia events are fired. Not all events types are tested, tell me if you want more. Upgraded PegDown library to last release
          Hide
          Lukas Theussl added a comment -

          Are you an apache committer? In that case you should have commit rights in the sandbox (I think). Tell me, otherwise I'll review the patch.

          Show
          Lukas Theussl added a comment - Are you an apache committer? In that case you should have commit rights in the sandbox (I think). Tell me, otherwise I'll review the patch.
          Hide
          Julien Nicoulaud added a comment -

          No, I'm not.

          Show
          Julien Nicoulaud added a comment - No, I'm not.
          Hide
          Lukas Theussl added a comment -

          Patch applied in r1128181, thanks! Some remarks:

          • I was stuck first with some weird plexus error until I realized that you had removed the text from MarkdownSink but not deleted the file
          • I also removed MarkdownParseException, as it only delegated to ParseException
          • please don't change the formatting, import order etc.

          Two more requests:

          • the test resource file test.md should contain a complete document with all supported markup, a la test.apt (it doesn't actually have to be tested, just to check that the parser doesn't choke on anything).
          • It would be nice to have a rudimentary site with intro and links to some external resources.

          Can you supply that? Otherwise looks good IMO.

          Show
          Lukas Theussl added a comment - Patch applied in r1128181 , thanks! Some remarks: I was stuck first with some weird plexus error until I realized that you had removed the text from MarkdownSink but not deleted the file I also removed MarkdownParseException, as it only delegated to ParseException please don't change the formatting, import order etc. Two more requests: the test resource file test.md should contain a complete document with all supported markup, a la test.apt (it doesn't actually have to be tested, just to check that the parser doesn't choke on anything). It would be nice to have a rudimentary site with intro and links to some external resources. Can you supply that? Otherwise looks good IMO.
          Hide
          Julien Nicoulaud added a comment -

          Sorry for the empty files, this is due to the way IntelliJ IDEA exports Subversion patches.

          Here is a new one:

          • test.md is now exhaustive.
          • Added a unit test I had forgotten.
          • Added rudimentary site index page.
          Show
          Julien Nicoulaud added a comment - Sorry for the empty files, this is due to the way IntelliJ IDEA exports Subversion patches. Here is a new one: test.md is now exhaustive. Added a unit test I had forgotten. Added rudimentary site index page.
          Hide
          Lukas Theussl added a comment -

          Patch applied in r1128829. Thanks!

          Show
          Lukas Theussl added a comment - Patch applied in r1128829 . Thanks!
          Hide
          Lukas Theussl added a comment -

          Moved out of the sandbox: r1129048. Thanks again!

          Show
          Lukas Theussl added a comment - Moved out of the sandbox: r1129048 . Thanks again!
          Hide
          Julien Nicoulaud added a comment -

          Great ! Thanks for the guidelines.

          Show
          Julien Nicoulaud added a comment - Great ! Thanks for the guidelines.
          Hide
          Lukas Theussl added a comment -

          Re-opening because of test failures: https://builds.apache.org/view/M-R/view/Maven/job/doxia/

          Apparently the pegdown dependency requires java 6, we are still stuck on java 5.

          Show
          Lukas Theussl added a comment - Re-opening because of test failures: https://builds.apache.org/view/M-R/view/Maven/job/doxia/ Apparently the pegdown dependency requires java 6, we are still stuck on java 5.
          Hide
          Julien Nicoulaud added a comment -

          This should have been fixed by pegdown 1.0.2 that just hit Central.

          Show
          Julien Nicoulaud added a comment - This should have been fixed by pegdown 1.0.2 that just hit Central.
          Hide
          Lukas Theussl added a comment -

          Committed again with upgraded pegdown dep: http://svn.apache.org/viewvc?rev=1143770&view=rev

          Jenkins is happy too: https://builds.apache.org/job/doxia/52/

          Just for the record: the dependency on xhtml-module should be removed if possible, delegating the work to another parser is not good practice. I opened DOXIA-436 for this.

          Show
          Lukas Theussl added a comment - Committed again with upgraded pegdown dep: http://svn.apache.org/viewvc?rev=1143770&view=rev Jenkins is happy too: https://builds.apache.org/job/doxia/52/ Just for the record: the dependency on xhtml-module should be removed if possible, delegating the work to another parser is not good practice. I opened DOXIA-436 for this.
          Hide
          Lukas Theussl added a comment -

          Closing with remarks above. Thanks!

          Show
          Lukas Theussl added a comment - Closing with remarks above. Thanks!

            People

            • Assignee:
              Lukas Theussl
              Reporter:
              Julien Nicoulaud
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development