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

Add support for table #56

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

antoineB
Copy link

No description provided.

@greghendershott
Copy link
Owner

Oh, wow, thank you very much. This is a significant PR. I'm going to need a little time to review it, but will try to do so today or over the weekend.

@@ -948,7 +1126,8 @@
(define $list (<or> $ordered-list $bullet-list))

(define $block
(<?> (<or> $atx-heading
(<?> (<or> $table
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be (unless-strict $table). Like for instance (unless-strict $verbatim/fenced) and other items in this list which add features not in the original markdown description. In other words if a user of the library sets current-strict-markdown? to #t, then tables should not be parsed.

@antoineB
Copy link
Author

Take your time to review, i am unsure of $table-cell many rule should be rewrite to stop fail on #\newline and on #| when it make sense.

Also check #57

@greghendershott
Copy link
Owner

i am unsure of $table-cell many rule should be rewrite to stop fail on #\newline and on #| when it make sense.

As for |: Although I haven't given it a lot of thought, I think you can add it to special-chars. Doing so might allow the rules to be simpler? (Adding | to special-char shouldn't break anything else unrelated to tables. As support for that, all the tests pass for me.)

@antoineB
Copy link
Author

Already implemented something, the idea is to parse everything expect #\newline #| (and special char) then reparse the result with a another parser.

@greghendershott
Copy link
Owner

I left a few comments. I might have a few more. Maybe we should touch base on the overall strategy.

  1. Do you want to keep making changes to the code based on my feedback? OR...
  2. Do you want me to take this and just make such changes myself?

I'm happy to proceed either way. It depends on your preference and how much time you want to spend on this.

Either way, I really appreciate your contribution. This is awesome. Thank you so much!

Add a condition to make table parsing unavailable in strict markdown.
Trim whitespace in table cell parse result.
Reuse the $inline parser to parse table cell.
@antoineB
Copy link
Author

I have applied your suggestions and agree with them.
I don't think the border or compact style should have any effect on the styling of the table.
And finally i have choice option 1 for now :) .

@greghendershott
Copy link
Owner

I have applied your suggestions and agree with them.

Thanks!

I don't think the border or compact style should have any effect on the styling of the table.

Ah, I misunderstood.

@antoineB
Copy link
Author

Ok, made the changes and the unit test remain clear, so a good code removing.

@greghendershott
Copy link
Owner

greghendershott commented Dec 2, 2015 via email

pmatos pushed a commit to pmatos/markdown-ng that referenced this pull request Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants