bhollis / maruku

A pure-Ruby Markdown-superset interpreter (Official Repo).
MIT License
502 stars 80 forks source link

Improved tables #81

Closed bprescott closed 11 years ago

bprescott commented 11 years ago

Added the ability to define table colspans using empty pipe symbols. If you want to span a column you just need to do the following:

   | h1        | h2  |   h3 |
   |:----------|:---:|-----:|
   |c1         | c2  |  c3  |
   |c1         | c2         ||
   |c1         ||       c2  |
   |c1                      |||

This change required some changes to the internal structure for the child nodes. I modified the needed tests and the latex generator to handle this internal change.

bhollis commented 11 years ago

Thanks Bill. That looks cool. Do you know if any other Markdown libraries do this?

bprescott commented 11 years ago

Hey Ben, There are at least two libraries that appear to support this feature. One is PHP Markdown-Extra (not verified by me) and the other is peg-markdown http://fletcher.github.com/peg-multimarkdown/mmd-manual.pdf.

I saw some discussion around this issue here http://comments.gmane.org/gmane.text.markdown.general/3841, that talked about a slightly different approach using measured tables but nobody could agree so I suppose it died. I would like to take the concept of measured tables and add that to maruku when I have some time, but that is a bigger fish than I can fry right now and the multi-pipe approach while not pretty does the job.

distler commented 11 years ago

I love this enhancement. +1 for merging it.

stomar commented 11 years ago

I'm not very happy about adding markdown extensions unless they are widely accepted.

Kramdown for example interprets those empty pipe symbols as empty table cells, which seems to be the most natural interpretation of the shown markdown, at least to me.

I need to work with different markdown interpreters, and I think it's better to have an unambiguous syntax that provokes an error message when used with the "wrong" interpreter, instead of having a syntax that might get silently misinterpreted.

distler commented 11 years ago

Interpreting the empty pipes as empty table cells seems like a quite reasonable fallback, for a markdown processor which doesn't support colspan's.

Here is how the proposed syntax is interpreted in the most popular Markdown processors.

As you can see, MultiMarkdown supports this syntax fully. PHP Markdown-Extra interprets the empty pipes as empty table cells. ... And the rest of the Markdown processors don't support even the previous table syntax (so Bill's additions don't make compatibility with them any worse).

(Note: In the example, rows 2,4 have colspan's; row 3 has an empty table cell. I think the difference is clear.)

distler commented 11 years ago

.P.S.: Here is another Markdown interpreter comparison site. It has more modern versions of a smaller number of Markdown interpreters. To be listed, it requires that maintainers of Markdown interpreters offer a web service. Not many have (e.g., Maruku is not there), which makes it somewhat less useful...

bhollis commented 11 years ago

My bad guys, I should have been clear on this. I think this is a cool syntax extension, but I'd like to think about it more before incorporating it into Maruku. I also don't want to make any syntax additions to 0.7.0 - I'm aiming for that release to be just bugfixes, and then put new features in 1.0.0. Does that sound OK? @bprescott, I'll move this onto a branch for inclusion into 1.0.0.

bprescott commented 11 years ago

No worries it's your call. I will leave my branch available to those who may want the feature until you decide when and if it is merged back in.

It could be argued that my extension was actually added to fix a bug. What I have proposed is a cleaner version of this syntax which should work in Maruku.

   | h1            |       h2  |        h3 |
   |:--------------|:---------:|----------:|
   |c1             | c2        |  c3       |
   |c1             |{:colspan="2"} c2      |
   |{:colspan="2"}  c1         |       c2  |
   |{:colspan="3"}       c1                |

It produces this HTML, which is unfortunately not what was expected.

<table>
  <thead>
    <tr><th>h1</th><th>h2</th><th>h3</th></tr>
  </thead>
  <tbody>
    <tr>
       <td style='text-align: left;'>c1</td>
       <td style='text-align: center;'>c2</td>
       <td style='text-align: right;'>c3</td>
    </tr>
    <tr>
       <td style='text-align: left;'>c1</td>
       <td style='text-align: center;' colspan='2'> c2</td>
       <td style='text-align: right;' colspan='2'> c1</td>
    </tr>
    <tr>
       <td style='text-align: left;'>c2</td>
       <td style='text-align: center;' colspan='3'> c1</td>
   </tr>
  </tbody>
</table>

Note that even adding empty pipes to the rows with the colspans to try and balance the rows will not work because Maruku swallows empty pipes.

So there was is a bug to fix and I feel that what I have proposed actually is more compatible with other markdown engines than the {:property="value"} approach in handling the usage of colspans in tables. My change also fixes this failure case so to me it's a win win.