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.

Sign up for GitHub

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

Jump to bottom

Fix table cells #1262

Merged
merged 8 commits into from
Jun 12, 2018
Merged

Fix table cells #1262

merged 8 commits into from
Jun 12, 2018

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented May 9, 2018 •
edited by styfle
Loading

Marked version: master

Markdown flavor: Markdown.pl|CommonMark|GitHub Flavored Markdown|n/a

Description

  • Fixes empty table cells and multiple slashes before |

fixes #1290

Contributor

  • Test(s) exist to ensure functionality and minimize regression

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@UziTech UziTech requested a review from davisjam May 9, 2018 21:27
UziTech
UziTech commented May 9, 2018
View reviewed changes
lib/marked.js
return ' |';
}
}),
cells = row.split(/ \|/),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally had a very complex replace regex but I couldn't get it to not be vulnerable to catastrophic backtracking, but this works

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer code to complex regexes anyway :-)

davisjam
davisjam reviewed Jun 2, 2018
View reviewed changes
test/specs/marked/marked.json Outdated
@@ -2,7 +2,31 @@
{
"section": "Code spans",
"markdown": "`someone@example.com`",
"html": "<p><code>someone@exmaple.com</code></p>\n",
"html": "<p><code>someone@exmaple.com</code></p>",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, and looks like a holdover, but "exmaple" -> "example" ?

davisjam
davisjam reviewed Jun 2, 2018
View reviewed changes
test/specs/marked/marked.json Outdated
"example": 1
},
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice set of tests. Could you add one or two really simple ones, e.g.

  • |1|
  • |1\||
  • |1\\1|

Starting simple makes the later ones easier to understand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still no example with a singly-escaped pipe? \|

davisjam
davisjam reviewed Jun 2, 2018
View reviewed changes
lib/marked.js Outdated
@@ -1340,7 +1340,20 @@ function merge(obj) {
}

function splitCells(tableRow, count) {
var cells = tableRow.replace(/([^\\])\|/g, '$1 |').split(/ +\| */),
var row = tableRow.replace(/\|/g, function (match, offset, str) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment above this line/function explaining its purpose? (It looks like the goal is to ensure that every cell-delimiting pipe has a space before it, so that you can split cells on ' |').

davisjam
davisjam reviewed Jun 2, 2018
View reviewed changes
lib/marked.js Outdated
@@ -1350,7 +1363,7 @@ function splitCells(tableRow, count) {
}

for (; i < cells.length; i++) {
cells[i] = cells[i].replace(/\\\|/g, '|');
cells[i] = cells[i].trim().replace(/\\\|/g, '|');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting in a comment that leading or trailing whitespace is ignored per the spec.

davisjam
davisjam requested changes Jun 2, 2018
View reviewed changes
Copy link
Contributor

@davisjam davisjam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, a few nits here and there.

@davisjam
Copy link
Contributor

davisjam commented Jun 2, 2018 •
edited
Loading

@UziTech

  1. Can you open an issue describing the problem?
  2. It seems like this PR fixes a spec compliance issue. Do any of the spec tests fail without the patch?

@UziTech
Copy link
Member Author

UziTech commented Jun 4, 2018

  1. What is the benefit of opening an issue? I feel like having an issue and PR will just fragment the conversation.

  2. All of the gfm Tables specs were passing but there were some tests that were not in the spec that would fail. (i.e. the tests I added)

    In addition I was trying to get rid of the vulnerable regex: / +\| */

@davisjam
Copy link
Contributor

davisjam commented Jun 4, 2018

@UziTech

  1. re: issue, this is my logic. Either a PR fixes a bug, or a PR adds new functionality. I am under the impression that this PR fixes a bug (although the absence of failing spec tests calls this into question?). Bugs deserve bug reports. Therefore there should be a bug report.

  2. Huh, this is surprising. Does this mean this is not a spec compliance issue?

@UziTech
Copy link
Member Author

UziTech commented Jun 5, 2018 •
edited
Loading

  1. In my view a PR is the same as an issue accept it has code associated with it. (i.e. this is the bug report) On GitHub I don't believe there is much of a difference between issues and PRs. You can even link to this PR with https://github.com/markedjs/marked/issues/1262

  2. Not exactly spec compliant, more like spec test compliant. It would work correctly if there was an escaped pipe (\|) in a table cell but not if there was an escaped slash right before a cell-delimiting pipe (\\|). It would assume any slash before a pipe meant the pipe was escaped.

@UziTech UziTech mentioned this pull request Jun 11, 2018
Closed
@UziTech UziTech requested review from styfle and joshbruce June 11, 2018 13:33
@UziTech
Copy link
Member Author

UziTech commented Jun 11, 2018

@davisjam if you need an issue #1290 is solved by this. I added a test for empty cells

@davisjam
Copy link
Contributor

@UziTech Haha good timing re: #1290.

@styfle styfle merged commit 05e322c into markedjs:master Jun 12, 2018
This was referenced Mar 30, 2020
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Apr 6, 2020
Open
Open
Open
Open
Open
Open
Open
Open
Closed
Closed
Open
Open
Closed
Open
Open
Open
Open
Open
Open
Open
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
@styfle
Merge pull request markedjs#1262 from UziTech/table-cells
e831e52 
Fix table cells
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davisjam davisjam davisjam approved these changes

@joshbruce joshbruce joshbruce approved these changes

@styfle styfle styfle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.5.0 - Commonmark Compliance
Development

Successfully merging this pull request may close these issues.

table render error
4 participants
@UziTech @davisjam @styfle @joshbruce