Implement MediaWiki table parsing

[davidswinegar]

(#10) These tables are just substitutes for HTML tags, so I used the existing Tag node with a few changes. This is a big pull request but I'm available to make any changes you would like to see. This also doesn't fix the issues from #55 regarding comments/templates as whitespace or handle implicitly closing tags from #40. Because the scope of those issues are greater than just tables I didn't implement them here.

One note: I also changed context in the CTokenizer to use uint64_t to ensure an exact width, which requires an include of <stdint.h>. I'm not sure how well this include works with VS2008 and VS2010, and I currently don't have access to a Windows computer, but it might be necessary in tokenizer.h to do something like:

#ifdef _WIN32
    typedef unsigned long long int uint64_t;
#else
    #include <stdint.h>
#endif

[earwig]

Okay! Good work, for starters. I'm actually really impressed that anyone managed to do this. Let's go through some various points:

• I'm okay with the <stdint.h> thing; I don't have a Windows computer either right now, so we'll wait until release time to see if we need to make any changes.
• It looks like there's a problem with building on python 3.4 specifically, according to Travis. I'll look into it.
• I'm going to merge this PR into feature/tables and review it there for a bit before merging into develop.

Will let you know if anything comes up.

[earwig]

In 94a9e32, I fixed a test (table_cell_unclosed_style) that wasn't being parsed correctly due to a missing comma. Now that the test is being run, it fails. Is this related to #40 as you mentioned above, and if so, should we mark it as an expected failure?

Another thing: why are <th>, <td>, and <tr> included as SINGLE tags?

[davidswinegar]

I've been testing on Python 2.7 so I'm not sure what the problem with 3.4
is. I just added that test this morning, didn't read the output after
seeing everything passed so my bad. That is related to #40 but the test
should actually fail as written if implicit close is implemented correctly.
I meant to test that style shouldn't cross cells which I apparently forgot
to implement (should just involve failing other contexts on cell end, not
to difficult to do). I won't be able to work on anything until tomorrow but
I can work on these problems and any others you find then.

On Monday, July 21, 2014, Ben Kurtovic notifications@github.com wrote:

In 94a9e32
94a9e32,
I fixed a test (table_cell_unclosed_style) that wasn't being parsed
correctly due to a missing comma. Now that the test is being run, it fails.
Is this related to #40
#40 as you mentioned
above, and if so, should we mark it as an expected failure?

—
Reply to this email directly or view it on GitHub
#81 (comment)
.

[earwig]

So I noticed that inline_cell_attributes doesn't roundtrip correctly:

>>> import mwparserfromhell
>>> text = u'{| \n ! name="foo bar" | test ||color="red"| markup!!foo | time \n|}'
>>> code = mwparserfromhell.parse(text)
>>> code == text
False
>>> print code
{|
 ! name="foo bar"|  test ||color="red"| markup!!foo | time
|}
>>> print text
{|
 ! name="foo bar" | test ||color="red"| markup!!foo | time
|}

Perhaps this needs to be re-ordered? I'm not sure if that would break anything else.

One final thing. I'm trying to understand the tree structure you're generating here, and it doesn't seem right to me. From the same example markup above, we get this:

>>> import re
>>> print re.sub("\s+", " ", code.get_tree())
< table > < th name = foo bar /> test < th color = red /> markup < th foo /> time \n </ table >

I asked above why <th> etc are allowed to be self-closing, because this doesn't seem correct. Surely the test, markup, and time strings should be inside their respective <th> tags, not between them?

[davidswinegar]

Back now and working on this, here are my thoughts:

tree structure
The tree structure is something I probably should have asked about. I realize now that the self closing tags don't enclose anything. I was confused because this is considered valid HTML:

<table>
  <tr>
    <td> foo
  <tr>
    <td> bar
</table>

This is the closest transliteration of wiki syntax to HTML table syntax, which is why I was trying to emulate it. But it doesn't make sense with the parse tree to treat them as self closing tags because then the table rows and table cells are separate from their contents. I think removing the self-closing bits and just setting the closing_wiki_markup to "" should do the trick, though it'll require some time to go back through the code to fix that.
This is also why the row and cell tags were included as single tags, because they are implicitly closed by the next row/cell in non-XHTML tables. As far as I can tell this isn't supported currently by mwparserfromhell.

Inline cell attributes
Yep, just a stupid mistake. I was thinking earlier that it might be useful to add an integration test that goes through all the mwtest examples and compares str(input) to str(wikicode) to catch these kinds of representation mistakes.

Travis/Python 3.4
I also see the CI tool building it as C90 while I was coding to C99 standards - it gave a few warnings because of that. It looks like VS doesn't support all of the C99 standard anyways so I'll look into changing any syntax that could be a problem.

Today I'll plan on going over the issues in this order:

• fix inline cell attributes
• kill style/templates/etc on table cell close + add tests (wontfix)
• switch table rows, cells, header to no longer be self-closing + change tests to match
• Python 3.4 issues, if I have time

[earwig]

Sorry, I've been dead lately due to a variety of real life things. Wanted to get on this before I leave for college (which is tomorrow...) but it doesn't look like that's going to happen. I'll take a look first thing after stuff gets settled down, which I expect to be within the next two weeks.

[earwig]

Alright, I'm back alive again and trying to work on this. My memory is a bit fuzzy from two months ago, so I'm going to try to figure out what still needs to be done for this:

• Add integration test for roundtripping existing unit test cases (done in bd85805)
• Investigate drop in coverage; see what code is untested

[earwig]

Okay! I'm reasonably happy with the code (for now), all tests are passing, and there are no obvious examples of stuff that's not covered (with the exception of |+ table captions, but ehh...), so I'm running some roundtripping tests on an enwiki dump and if all goes well, I'm going to merge this tonight.

Edit: >500,000 pages tested, about 10% with tables, all roundtripped correctly so far. Going to sleep now, will merge tomorrow assuming nothing changes.

Edit 2: Noticed a bug on Natasha Kaplinsky. Will try to fix.

Edit 3: There is definitely a bug in how the code is handling closing pipes when present on the same line as the open. For example:

{| |}
| foo
|}

MediaWiki treats this as a table with a single cell "foo" (the first |} is ignored), while this code treats it as an empty table with the | foo \n|} treated as regular text. I suppose it's enough of an edge case to ignore for now, but I still need to fix the roundtripping bug present in the article above.

Edit 4: The syntax that looks like {| |} (close on same line as open) is actually not a valid table closure. I'm going to change the code to ignore this, which should fix the above problem.

Edit 5: Pushed updates; will re-run roundtripping tests.

Edit 6: There's a problem with Rafael Nadal but after several hours on this one I can't figure it out. I refactored a lot of the table error recovery code, but that doesn't seem to have fixed it. I want to blame #42, but I'm not sure if that's the real cause here.

[coveralls]

Coverage decreased (-0.2%) when pulling a15f617 on davidswinegar:tables into 810c24e on earwig:develop.

[davidswinegar]

Just saw your edits, that's very weird - I was testing against syntax on test pages and the "close on same line as open" behavior was making valid tables, I guess it's not supposed to be. The Nadal page is definitely hitting the max cycles on my machine, maybe #42 is to blame but I'm not sure, I can take a closer look as well this weekend.

[earwig]

Okay, gonna go ahead and merge this. The remaining issue re: Nadal can be dealt with separately.

[legoktm]

Exactly 666 commits!