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

Remove nptable tokenizer #2126

Closed
wants to merge 2 commits into from

Conversation

calculuschild
Copy link
Contributor

@calculuschild calculuschild commented Jul 2, 2021

nptable tokenizer is 95% identical to table, except for handling a weird edge case. This PR moves that special case to be handled in splitCells() to reduce code redundancy and improve speed (one less tokenizer to cycle through).

Also corrects one new test that did not follow the correct behavior compared to actual Github behavior: If a row of cells is missing the leading\trailing pipe, then the leading\trailing cell cannot be empty.

(this is also just some cleanup in prep for #2124, since moving things into the Tokenizer was resulting in large duplicated blocks in each both table and nptable)

 A | B
-- | --
   | 2

Results in:

A B
2

Current Master Benchmark

es5 marked completed in 4795ms and passed 86.66%
es6 marked completed in 4803ms and passed 86.66%
es5 marked (gfm) completed in 5162ms and passed 86.04%
es6 marked (gfm) completed in 5227ms and passed 86.04%
es5 marked (pedantic) completed in 4980ms and passed 70.86%
es6 marked (pedantic) completed in 5040ms and passed 70.86%

This PR Benchmark

es5 marked completed in 4673ms and passed 86.66%
es6 marked completed in 4777ms and passed 86.66%
es5 marked (gfm) completed in 4850ms and passed 86.04%
es6 marked (gfm) completed in 4985ms and passed 86.04%
es5 marked (pedantic) completed in 4774ms and passed 70.86%
es6 marked (pedantic) completed in 4922ms and passed 70.86%

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

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

nptable is 95% identical to table, except for handling a weird special case.  Special case now handled in splitCells()
@vercel
Copy link

vercel bot commented Jul 2, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/markedjs/markedjs/4oXnciCGLtV8qWTA9UbueQMZPyoe
✅ Preview: https://markedjs-git-fork-calculuschild-removenptable-markedjs.vercel.app

@calculuschild calculuschild added category: tables RR - refactor & re-engineer Results in an improvement to developers using Marked, or end-users, or both. labels Jul 2, 2021
@UziTech
Copy link
Member

UziTech commented Jul 3, 2021

If a row of cells is missing the leading\trailing pipe, then the leading\trailing cell cannot be empty.

Is this in the spec or just unspecified behavior that gfm seems to get wrong?

@calculuschild
Copy link
Contributor Author

It's not in the GFM spec one way or the other, but after playing around a lot with the Github parser, that appears to be the rule. So this is more "replicating what Github's parser does".

@calculuschild
Copy link
Contributor Author

Since this removes an entire Tokenizer, I presume this would be breaking? Is there a standard procedure for grouping multiple breaking PRs together so they can all go out in the same major version bump? (#2126, #2124 , #2112)

@UziTech
Copy link
Member

UziTech commented Jul 3, 2021

When I merge them I just cancel the release in CI until the last one.

@calculuschild
Copy link
Contributor Author

Integrated into #2124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tables RR - refactor & re-engineer Results in an improvement to developers using Marked, or end-users, or both.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants