Skip to content

Fix pipe table parsing with a leading paragraph#905

Merged
xoofx merged 2 commits intoxoofx:masterfrom
MihaZupan:table-para
Oct 20, 2025
Merged

Fix pipe table parsing with a leading paragraph#905
xoofx merged 2 commits intoxoofx:masterfrom
MihaZupan:table-para

Conversation

@MihaZupan
Copy link
Collaborator

@MihaZupan MihaZupan commented Oct 20, 2025

Closes #818
Addresses issues referenced in #885 (comment), #885 (comment)

This PR effectively reverts #885 as that change was swallowing whatever information was there in the paragraph before the table.
E.g. for

Some text
| A |
|---|
| B |

"Some text" would be lost.

Instead I relaxed the table validation to no longer enforce starting on the first line (localLineIndex > 0 and deltaLine > 0 checks), and also save the current block in the AST before we replace it with a table.

Given this is happening while we're parsing inlines, I don't think we can easily insert an extra block at the current layer, so I instead added the leading paragrah as the first child of the table, and the first element is special-cased in the renderer.
That is, instead of

document
  paragraph
    some text
  table
    A ...

we're instead producing

document
  table
    paragraph
      some text
    A ...

Since TableRow is a public type, this does risk breaking code that was walking the AST and assuming that all children of a Table are TableRows - this seems plausible given our own renderer did that.
I'm open to alternative suggestions.

@MihaZupan MihaZupan added the bug label Oct 20, 2025
@MihaZupan
Copy link
Collaborator Author

MihaZupan commented Oct 20, 2025

Alternative approach that'd get us the expected syntax tree shape and avoids modifying the renderer: master...MihaZupan:table-para2
This being a hack that says "insert the block, but skip processing it in MarkdownParser.ProcessInlines".

Could theoretically break someone that wanted to look at the table shape from a different parser (since the table isn't inserted until later), but that seems less likely?

@xoofx
Copy link
Owner

xoofx commented Oct 20, 2025

Could theoretically break someone that wanted to look at the table shape from a different parser (since the table isn't inserted until later), but that seems less likely?

Yes, maybe that workaround is ok. Table parsing is a bit awful unfortunately, so this new code doesn't change that 😅

@xoofx xoofx merged commit d6e88f1 into xoofx:master Oct 20, 2025
1 check passed
@RickStrahl
Copy link
Contributor

Can the same logic that's used for GridTable parsing to find the start of the block? GridTables seem to have the correct behavior.

ps. I haven't had time to look at the code beyond what was posted here, just going on the rendering behavior in the current 0.42 release.

@MihaZupan
Copy link
Collaborator Author

Maybe.
Grid tables are parsed with a block parser, while the pipe table is parsed with an inline one that effectively hijacks a paragraph block and replaces it with a table block.

I haven't been bored enough yet to look at what it'd take to turn it into a block parser instead 😄
Maybe Alexandre remembers more details.

@RickStrahl
Copy link
Contributor

Yeah I know what you mean - I took a look at the code a few days ago thinking I could help in a hurry, but without setting aside a good chunk of time to understand the flow these block parsers are easy to mess up. I made a custom fix a few years ago and discovered months later it was breaking in completely unexpected ways.

Thanks for your effort here - really appreciate you taking the time!

Aloha.

@MihaZupan
Copy link
Collaborator Author

Note that this change makes it work when you have leading text before the table.
If you have text right after the table, that still won't work.

tsuereth added a commit to tsuereth/glog that referenced this pull request Nov 9, 2025
NOTE: This doesn't include the current newest `Markdig` version 0.43.0, as this has introduced some rendering issues with pipe tables.

- A fix made for "tables with leading paragraphs" results in *adding* a leading paragraph to tables which shouldn't have one. xoofx/markdig#905

Notable changes in the test project:

- `MSTest.TestFramework` upgrades deprecated and removed the "ExpectedException" attribute, in favor of `Assert.Throws...()` test assertions.
  - It's also deprecated assertions on enumerable/collection Count values, in favor of countable assertion methods like `IsEmpty()` and `HasCount()`.
  - AND, its analyzer now throws an error if there's no explicit Parallelize setting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tables are not rendered properly unless there is a new line before them

3 participants