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.js for pipes inside backticks #697

Closed
wants to merge 1 commit into from
Closed

Fix tables.js for pipes inside backticks #697

wants to merge 1 commit into from

Conversation

klassiker
Copy link

Add test samples and fix wrong test sample

The sample Pipes inside backticks don't split cells did not contain the output that github would create from that markdown.

 | Heading 1 | Heading 2
 | --------- | ---------
 | Cell 1    | Cell 2
 | `Cell|3`  | Cell 4

produces

Heading 1 Heading 2
Cell 1 Cell 2
`Cell 3`

If the current cell is backTicked, the current char is a pipe and the previous char is not an escape, the cell ends.

After adjusting that it is no longer necessary to check if the end has been reached to due an unmatched backtick.

Add test samples and fix wrong test sample
@puzrin
Copy link
Member

puzrin commented Aug 1, 2020

Thanks for PR. All pending issues will be reviewed in 1-2 weeks. Sorry for delay.

@spencer246
Copy link

I am not sure whether table.js should deal with backticks at all.

#86 made backticked pipes in table cells not parsed as delimiters. Back then, it was consistent with GFM. However, after 2 years later, GFM changed their spec; see github/cmark-gfm#24 and dicussion therein. GFM now does parse every | as a table delimiter unless it is escaped.

Current implementation of table.js has another issue: #689 (e.g., the case in which a table cell contains complicated backtick sequences like `` ` ``), which will also be fixed if table.js does not deal with backticks at all.

@puzrin
Copy link
Member

puzrin commented Aug 30, 2020

@barro350 thanks for reference. We will sync with spec. Tables are intended be close to github. But implementation was done before spec available.

NOTE. This will be breaking change.

@rlidwka rlidwka closed this in c61f105 Sep 14, 2020
@rlidwka
Copy link
Member

rlidwka commented Sep 14, 2020

I've changed table.js to be similar to up-to-date gfm:

  • table now doesn't deal with backticks at all
  • | is considered escaped if and only if there is a \ character immediately before it
  • number of elements in the first row (thead) now must match second row (aligns) exactly
  • no tbody if it would be empty

I believe it fixes this issue (all your tests pass), so closing this PR.

rlidwka added a commit that referenced this pull request Sep 14, 2020
 - table now doesn't deal with backticks at all
 - `|` is considered escaped if and only if there is a `\` character immediately before it
 - number of elements in the first row (thead) now must match second row (aligns) exactly
 - no tbody if it would be empty

close #689
close #697
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.

None yet

4 participants