Skip to content
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

One-cell table doesn't parse on v2.6.8 #539

Closed
jaumef opened this issue Jan 26, 2017 · 12 comments
Closed

One-cell table doesn't parse on v2.6.8 #539

jaumef opened this issue Jan 26, 2017 · 12 comments

Comments

@jaumef
Copy link

jaumef commented Jan 26, 2017

Hi there, I'm using python markdown along with other markdown extensions.

My tests failed with version 2.6.8, with one-cell tables not being parsed

E.i.:

|  I'm a table  |
|---------------|

It may be a bug with our extensions, can you provide further information in your changelog?

Thank you :)

PD: With version 2.6.7 all works fine 👍

@facelessuser
Copy link
Collaborator

facelessuser commented Jan 26, 2017

You know, when I refactored tables, there were no tests for this situation. I had compared here to see what Python Markdown historically did, and, at least in 2.6.5, it didn't check for this. I didn't know, and didn't specifically check, that 2.6.7 handled this. It wasn't purposefully added as no tests for it got added at any time to ensure this didn't get broken. I imagine it was a side affect of other changes.

With that said, PHP Markdown Extra, which this extension was based off, appears to specifically state this single column scenarios. So it looks like I didn't do too good at my homework. In context to PHP markdown, this would appear to be a bug to me.

@jaumef
Copy link
Author

jaumef commented Jan 26, 2017

Ok! I'll be staying in the v2.6.7 untill this gets fixed!

Thank you for your fast answer 👍 😄

@waylan
Copy link
Member

waylan commented Jan 26, 2017

The way I read PHP Markdown Extra's documentation I would not expect that to work. It speaks of a one column table. This is one header cell only. To my surprise, it does parse as a table by PHP Markdown Extra:

<table>
<thead>
<tr>
  <th>I'm a table</th>
</tr>
</thead>
<tbody>
<tr>
  <td></td>
</tr>
</tbody>
</table>

Notice that the data is in the header and an empty cell is created below the header. This is probably not what you want, but if we support this scenario, it is the only solution we will consider. Personally, I think it should not be considered a table as it contains no data cells. If it worked before, that was probably not intentional.

@facelessuser
Copy link
Collaborator

In general we are not handling single column all anymore.

This would fix it:

    def test(self, parent, block):
        """
        Ensure first two rows (column header and separator row) are valid table rows.

        Keep border check and separator row do avoid repeating the work.
        """
        is_table = False
        rows = [row.strip() for row in block.split('\n')]
        if len(rows) > 1:
            header0 = rows[0]
            border_side = 0
            self.border = False
            if header0.startswith('|'):
                self.border = True
            elif self.RE_END_BORDER.search(header0) is not None:
                self.border = True
                border_side = 1
            row = self._split_row(header0)
            row0_len = len(row)
            is_table = row0_len > 1

            # Check for single column table by
            # Esuring there is at least a single pipe either
            # Preceding or trailing all rows;
            # and they need to be on the same side.
            if not is_table and row0_len == 1 and self.border:
                for index in range(1, len(rows)):
                    if border_side:
                        is_table = self.RE_END_BORDER.search(rows[index]) is not None
                    else:
                        is_table = rows[index].startswith('|')
                    if not is_table:
                        break

            if is_table:
                row = self._split_row(rows[1])
                is_table = (len(row) == row0_len) and set(''.join(row)) <= set('|:- ')
                if is_table:
                    self.separator = row

        return is_table
Single column tables

| Is a Table
| ----------

| Is a Table
| ----------
| row

Is a Table |
---------- |

Is a Table |
---------- |
row        |

Is a Table |
---------- |

Is not a Table |
-------------- |
| row

Is not a Table
-------------- |
row            |

Is not a Table |
-------------- |
row            |
row
<p>Single column tables</p>
<table>
<thead>
<tr>
<th>Is a Table</th>
</tr>
</thead>
<tbody></tbody>
</table>
<table>
<thead>
<tr>
<th>Is a Table</th>
</tr>
</thead>
<tbody>
<tr>
<td>row</td>
</tr>
</tbody>
</table>
<table>
<thead>
<tr>
<th>Is a Table</th>
</tr>
</thead>
<tbody></tbody>
</table>
<table>
<thead>
<tr>
<th>Is a Table</th>
</tr>
</thead>
<tbody>
<tr>
<td>row</td>
</tr>
</tbody>
</table>
<table>
<thead>
<tr>
<th>Is a Table</th>
</tr>
</thead>
<tbody></tbody>
</table>
<p>Is not a Table |
-------------- |
| row</p>
<p>Is not a Table
-------------- |
row            |</p>
<p>Is not a Table |
-------------- |
row            |
row</p>

@jaumef
Copy link
Author

jaumef commented Jan 26, 2017

@waylan Ok! I see your point in there.
The way I'm using it might not be the one you'll hope for a "basic table", so I'll be adapting my content to it or find an alternative way.

@facelessuser thank you for the code.

I'll be considering an alternative. Thank you for your support 😄 👍

@facelessuser
Copy link
Collaborator

I'd probably need to remove the single cell case, or inject an extra row.

@facelessuser
Copy link
Collaborator

@waylan, Let me know your preference for body-less tables, and I'll send a pull to address single columns and the tables without bodies (either exclude or inject an empty body). At the very list, I should get proper single tables fixed.

@facelessuser
Copy link
Collaborator

I'll probably just monkey Extra unless told otherwise.

@waylan
Copy link
Member

waylan commented Jan 26, 2017

I have always maintained that we follow PHP Markdown Extra's table syntax as close as reasonably possible. And I just checked and an empty <tbody> is invalid HTML. So let's add the empty row/cell.

@facelessuser
Copy link
Collaborator

Will do. Kicking myself for the single column regression. Just assumed it was invalid as suddenly border pipes would have to matter. Mental note, always references the model spec.

@waylan
Copy link
Member

waylan commented Jan 26, 2017

@jaumef there are a few alternate table extensions listed on the wiki. There might be one which would better serve your needs. I believe at least one of them does not require a header, which should give you your single cell. Although, if you only need a single cell, then you probably are not using it for tabular data so an alternate approach may be better. If you are looking for a way to put some content in a box, perhaps the admonition extension would serve you better.

@jaumef
Copy link
Author

jaumef commented Jan 26, 2017

@waylan @facelessuser Thank you so much both of you ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants