search

greghendershott / markdown

Markdown parser written in Racket.
101 stars 28 forks source link

Add support for table #56

Open antoineB opened 8 years ago

greghendershott commented 8 years ago

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.

antoineB commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

greghendershott commented 8 years ago

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!

antoineB commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

greghendershott commented 8 years ago

Thanks! I'm excited to look at it, but unfortunately might not be able to for a few days.