RFC add markdown Table #9878

hayd · 2015-01-22T09:01:53Z

This parses and displays tables in Markdown with GH syntax, in the terminal and html. I haven't tested the latex version and the plain is not yet finished.

As discussed in other documentation issues, tables are useful to have...

cc @one-more-minute, fixes JuliaAttic/Markdown.jl#16.

No doubt there are a few things I'm not doing correctly here, so comments would be great.

~~Note: There aren't any markdown tests at all...~~

MikeInnes · 2015-01-22T17:45:35Z

Nice! Great to see this.

After a skim of the code it looks pretty reasonable, but I'll need to play around with it a bit. I'll try to get that done within a few days.

hayd · 2015-01-22T18:12:12Z

base/markdown/render/plain.jl

+    elseif align == :r
+        return "-" ^ max(1, width - 1) * ":"
+    elseif align == :c
+        return "-" ^ width


Hmm this should be ":" * "-" ^ max(1, width - 2) * ":", since default is to right align.

hayd · 2015-01-22T18:41:13Z

@one-more-minute thanks for looking, I just pushed a commit to simplify (IOBuffer with SubString) but in retrospect it doesn't work, will see if I can fix it, or will just delete that commit. Removed it!

Edit: that is, the simplifying commit doesn't work, the first/current implementation seems fine.

hayd · 2015-01-22T22:16:30Z

base/markdown/GitHub/GitHub.jl

+_full(s::AbstractString) = s
+
+@flavor github [list, indentcode, blockquote, fencedcode, hashheader,
+                github_paragraph, github_table,


it may be that table needs to be earlier in this list, is this basically precedence?

Yup, it's precedence. If the table could be interpreted as another block element (such as a paragraph) it needs to go before that element (and in order of efficiency otherwise). I imagine that swapping table and paragraph here would be the right thing. You'll want to also have the same order in the julia flavour.

I put it first in the julia @flavor and it seemed to work, will update (I think being before paragraph is the important thing).

I suspect there may be some edge cases - are precedence for markdown parsing published somewhere? We really need some tests here IMO (will put a few together in another PR)...

hayd · 2015-01-22T23:20:39Z

@one-more-minute updated precedence and fixed the plain printing. I've not checked latex, not sure how to call it.

MikeInnes · 2015-01-24T12:59:14Z

RE tests, feel free to add a Markdown tests file if you want. I'll try and get equality working to make this easier.

FWIW, GitHub's Markdown parser appears to be a bit stricter than this code. At least three dashes are required, and if you're using an outer pipe it mustn't be indented (so the example below wouldn't parse). I'm not strongly opinionated about what the "right" way here is, so if you're happy with the current behaviour I'm cool with that, but in might be worth bearing in mind (there may be a good reason for it).

Inline formatting throws an error when rendering, currently:

julia> md"""
       | a | b | c |
         | - | - | - |
       | *d* | e | f |
       """
Error showing value of type Base.Markdown.MD:
ERROR: MethodError: `replace` has no method matching replace(::Base.Markdown.Italic, ::Regex, ::ASCIIString)
 in ansi_length at markdown/render/terminal/formatting.jl:48
 in ansi_length at markdown/render/terminal/render.jl:87

I'm guessing you just need to render to a string via plaininline/terminline before calling ansilength.

I'll take a look over the code itself and add any other comments I can think of.

@MichaelHatherly any chance you could have a quick look over the LaTeX code?

MikeInnes · 2015-01-24T13:01:36Z

base/markdown/GitHub/GitHub.jl

@@ -33,7 +33,83 @@ function github_paragraph(stream::IO, md::MD, config::Config)
    return true
 end

-# TODO: tables
+typealias Row Vector


I think it'd be best to make this explicitly Vector{Any}, or you risk running into subtle typing errors.

Since it's only used once anyway, I wouldn't mind if it was inlined.

MikeInnes · 2015-01-25T09:26:02Z

base/markdown/render/plain.jl

+plaininline(md::Code) = "`$(md.code)`"
+
+plaininline(io::IO, md::Vector) = plaininline(io, md...)
+plaininline(io::IO, md::Union(Image, String, Bold, Italic, Code, Link, Image)) = print(plaininline(md))


Constructing the string is going to be slow here, so it's best to do it the other way around – leave the plaininline(io... methods as they are and have a plaininline(x) = sprint(plaininline, x) method if it's not there already.

MikeInnes · 2015-01-25T14:19:18Z

Ok, this is coming along nicely, a little bit of polish and we should be good to go.

I've improved the way terminal formatting is done, so if you rebase this PR against master you should be able to use prettier output without any trouble (but let me know if you come across anything).

The other thing is: Since tables are fairly complex elements that aren't part of standard markdown, I think it would be best not to mix this code with the other elements. Let's have a table.jl file (with all the table parsing and rendering code) next to github.jl and include it there.

The other thing is that there's a build failure going on, not sure why though. Rebasing might help.

hayd · 2015-01-25T16:01:22Z

thanks for the feedback, that all sound good.

Let's have a table.jl file (with all the table parsing and rendering code)

Just to clarify, extract parsing to Github/table.jl, i think i should make a new file for rendering too in render/table.jl (this seems convention, and as in future Table may not be gh specific) ?

MikeInnes · 2015-01-25T16:27:16Z

I meant put everything in GitHub/table.jl. It's only a hundred lines or so, but it's complex enough that I think making it really self-contained will be much easier to follow.

hayd · 2015-01-26T04:48:05Z

@one-more-minute moved to table.jl, have a final refactor of plain (as you suggested). Adding some tests and I came across the following strange behaviour, any ideas on how to fix? (does it matter?)

julia> m = Markdown.parse("""a|b
       1|2""")
  a    b
---  ---
  1    2

julia> 1, m
(1,Markdown.MD(Any[Markdown.Table([Any[Any["a"],Any["b"]],Any[Any["1"],Any["2"]]],:r)],Dict{Any,Any}()))

julia> Markdown.MD(Any[Markdown.Table([Any[Any["a"],Any["b"]],Any[Any["1"],Any["2"]]],:r)],Dict{Any,Any}())
  'a'
---
  'b'
  '1'
  '2'

Note: it works fine if you prepend the first array with Any

julia> Markdown.MD(Any[Markdown.Table(Any[Any[Any["a"],Any["b"]],Any[Any["1"],Any["2"]]],:r)],Dict{Any,Any}())
  a    b
---  ---
  1    2

@MichaelHatherly The latex is commented out in the Markdown.jl file (i.e. it's not in base atm) and it seems to have missed a refactor after Block/Inline API change (I'm not sure if the outcome of JuliaAttic/Markdown.jl#5 is when @one-more-minute?)... IMO inclusion of Inline, Block and Content types looked pretty useful, would have been neat in the last few changes I've been doing (even if they are just Unions). Presumably there was a good reason to ditch them?

Edited: Above paragraph, which was especially confusing...

MikeInnes · 2015-01-26T09:23:43Z

test/markdown.jl

+                            Any[Any["d",Markdown.Code("","gh"),"hg"],
+                                Any["hgh",Markdown.Bold(Any["jhj"]),"ge"],
+                                Any["f"]]],
+                        [:l, :r, :r]))


It would be nice to make the tests as human-readable as possible, e.g. you can write this as

MD(Table(Any[["a","b","c"], Any[["d",Code("gh"),"hg"], ["hgh",Bold("jhj"),"ge"], "f"]], [:l, :r, :r]))

(This actually doesn't work at the moment, which I think is due to a problem with print_align)

MikeInnes · 2015-01-26T09:37:50Z

The reason you sometimes need Any[... is that [...] concatenates by default, whereas T[...] doesn't. e.g.

x = [1,2,3]
y = [4,5,6]
[x, y]
[x y]

This behaviour is fairly likely to change in future, I think.

hayd · 2015-01-28T19:37:32Z

@one-more-minute I think I've addressed you comments for this and for the sections PR. Would be great to get these merged! :)

I want to import, and fix up, latex (currently not imported at all) and test it another PR, as well as some other stuff, but am waiting on these two.

hayd · 2015-01-31T08:09:22Z

bump!

ViralBShah · 2015-01-31T09:04:46Z

Could you squash the commits here too?

MikeInnes · 2015-01-31T10:31:20Z

@hayd Ok, I finally have some time today – let me know when you've squashed, I'll merge and do some of the refactoring.

MikeInnes · 2015-01-31T10:53:31Z

Actually, since you're probably asleep I'll do some rebasing for you, if that's OK. That way you won't have to wait until next weekend again ;)

MikeInnes self-assigned this Jan 22, 2015

hayd reviewed Jan 22, 2015
View reviewed changes

hayd force-pushed the markdown_table branch from 8cd5310 to f70f685 Compare January 22, 2015 19:52

hayd reviewed Jan 22, 2015
View reviewed changes

MikeInnes reviewed Jan 24, 2015
View reviewed changes

hayd force-pushed the markdown_table branch from 9607fc2 to ee27d42 Compare January 24, 2015 23:16

MikeInnes reviewed Jan 25, 2015
View reviewed changes

hayd added 4 commits January 25, 2015 09:26

ENH add markdown Table

9d60cf0

FIX plain markdown printing, and precedence for Github

b4b8324

clean up markdown table based on @one-more-minute comments

c752ffd

fix use of sprint and escapes mispelling

fc60fb8

move markdown table code to its own file

d36e51e

hayd force-pushed the markdown_table branch from ee27d42 to d36e51e Compare January 25, 2015 19:34

test markdown table

575cf67

MikeInnes reviewed Jan 26, 2015
View reviewed changes

remove ansi_length calls

cca79f6

MikeInnes added a commit that referenced this pull request Jan 31, 2015

merge #9878

ed4a227

MikeInnes closed this Jan 31, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC add markdown Table #9878

RFC add markdown Table #9878

hayd commented Jan 22, 2015

MikeInnes commented Jan 22, 2015

hayd Jan 22, 2015

hayd commented Jan 22, 2015

hayd Jan 22, 2015

MikeInnes Jan 22, 2015

hayd Jan 22, 2015

hayd commented Jan 22, 2015

MikeInnes commented Jan 24, 2015

MikeInnes Jan 24, 2015

MikeInnes Jan 25, 2015

MikeInnes commented Jan 25, 2015

hayd commented Jan 25, 2015

MikeInnes commented Jan 25, 2015

hayd commented Jan 26, 2015

MikeInnes Jan 26, 2015

MikeInnes commented Jan 26, 2015

hayd commented Jan 28, 2015

hayd commented Jan 31, 2015

ViralBShah commented Jan 31, 2015

MikeInnes commented Jan 31, 2015

MikeInnes commented Jan 31, 2015

RFC add markdown Table #9878

RFC add markdown Table #9878

Conversation

hayd commented Jan 22, 2015

MikeInnes commented Jan 22, 2015

hayd Jan 22, 2015

Choose a reason for hiding this comment

hayd commented Jan 22, 2015

hayd Jan 22, 2015

Choose a reason for hiding this comment

MikeInnes Jan 22, 2015

Choose a reason for hiding this comment

hayd Jan 22, 2015

Choose a reason for hiding this comment

hayd commented Jan 22, 2015

MikeInnes commented Jan 24, 2015

MikeInnes Jan 24, 2015

Choose a reason for hiding this comment

MikeInnes Jan 25, 2015

Choose a reason for hiding this comment

MikeInnes commented Jan 25, 2015

hayd commented Jan 25, 2015

MikeInnes commented Jan 25, 2015

hayd commented Jan 26, 2015

MikeInnes Jan 26, 2015

Choose a reason for hiding this comment

MikeInnes commented Jan 26, 2015

hayd commented Jan 28, 2015

hayd commented Jan 31, 2015

ViralBShah commented Jan 31, 2015

MikeInnes commented Jan 31, 2015

MikeInnes commented Jan 31, 2015