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

Fix tables with leading/trailing header pipes and trailing spaces #244

Conversation

bplump
Copy link
Contributor

@bplump bplump commented Sep 5, 2021

This is another table rendering issue introduced in 0.16.1. It's extremely obscure (found by running 0.18.0 against millions of markdown comments and comparing against 0.15.2) but I'm hoping the fix is simple enough that we can maintain compatibility with 0.15.2. 🤞

It's probably easiest to look at the added test cases - note the space at the end of the header line in the first test. When the header row has leading/trailing pipes and the trailing pipe is followed by whitespace, the table fails to render. (Trailing whitespace on the separator line or content lines works fine; trailing whitespace when the header line doesn't have a trailing pipe also works fine)

The 0.15.2 code would trim each line at the start of split(). Without the trim, split() adds the whitespace after the trailing pipe as another header cell. As a result, tryStart thinks there are more header cells than columns and stops trying to parse a table.

The current code already removes any leading whitespace and any leading pipe. If and only if there's a leading pipe, the fix does the same for the trailing pipe.

@bplump bplump force-pushed the fix-table-heading-with-pipes-and-trailing-spaces branch from d1ac104 to cb692b9 Compare September 5, 2021 13:57
@codecov-commenter
Copy link

Codecov Report

Merging #244 (cb692b9) into main (726d369) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main     #244   +/-   ##
=========================================
  Coverage     94.20%   94.21%           
  Complexity      220      220           
=========================================
  Files           124      124           
  Lines          3588     3593    +5     
  Branches        542      542           
=========================================
+ Hits           3380     3385    +5     
  Misses          104      104           
  Partials        104      104           
Impacted Files Coverage Δ
...mark/ext/gfm/tables/internal/TableBlockParser.java 98.63% <100.00%> (+0.04%) ⬆️

@robinst robinst merged commit e3a50fc into commonmark:main Sep 6, 2021
@robinst
Copy link
Collaborator

robinst commented Sep 6, 2021

found by running 0.18.0 against millions of markdown comments and comparing against 0.15.2

Love it!

Thanks for the fix, I'll do a release with this shortly.

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

Successfully merging this pull request may close these issues.

3 participants