Move markdown types into their own file.

[hayd]

cc @one-more-minute, I know we discussed this, but after hacking on some other bits, it seems clearer that this is a significant improvement in terms of clearer code / simplicity.... Also, I realised that (after doing this as a proof of concept) it's is going to be difficult to rebase and I don't really want to do it again... so selfishly wanted to punt it as a PR. :)

All this does is reorder methods so that the rendering utils are available when markdown types are defined, e.g. group all the methods related to Code together.

This also adds in a couple of missing methods (which I spotted were missing once in individual files).

Edit: I don't think the linux32 travis failure is me!

[hayd]

@one-more-minute I rebased and cleaned up a bit. :)

[hayd]

rebased!

[tkelman]

This should be more consistent about the license headers

[hayd]

@tkelman Ooops, I've prepended these to all the new files.

[tkelman]

Not a fan of so many tiny, deeply nested files.

[hayd]

I've not changed the depth, but the current naming is incorrect (distinguishing between block and inline - was from an earlier iteration of Markdown.jl).

Note: that the grouping (types with their display methods) is a definite improvement as/and this PR actually uncovered a couple of missing definitions (which were obvious once they were next to each other). So I think we should do some moving here, if there's a better solution I'm open to it...

What's the logic to preferring larger files?

[tkelman]

It's just a lot of clutter and not a lot of content when files are tiny. Too much editor fiddling and moving back and forth between files, when really these are just part of a markdown parser. There's a non-negligible cognitive and organizational cost per source file, but certainly also a point where files get too large and including too much content that they should be split up. It's subjective, but personally I think the sweet spot is somewhere over 150ish lines to be worth a separate file and somewhere under 1000 to be reasonably well-factored.

For 0.5 or sometime later we should really reconsider whether the markdown parser needs to be in base or if it can be an optional presentation-only feature as an independently-developed module, so code organization questions like this don't really have to be a review burden on either base or the markdown functionality.

[hayd]

I disagree that this is a "review burden", that's not the reason it's not been merged. It's a style thing just like you attested. Talk of moving from base, whilst true seems premature/irrelevant to me as, right now, it's in base... in semantically incorrect files (which caused some confusion for at least me).

non-negligible cognitive and organizational cost per source file

In general this is true, in this case it's not, since after this PR each file contains exactly the same thing (a type, how it's parsed and how it's rendered)... Right now if you want to learn how List is parsed and rendered you need to look in 5+ files.

Like I say, there are alternatives e.g. bring all common types into one file... however, IMO that's just trying to hit the "sweet spot" for the sake if it, rather than for greater code clarity.

[MikeInnes]

FWIW, I like this organisation in general I'm in favour of smaller files rather than thousand-line behemoths. But this does strike me as a little overly fine grained as well.

Style changes like this are always a bit tricky because everyone has a different way of doing it. Rather than making you go through the minutiae of what to put where, maybe it's best if I take a quick go at this myself and put something up to see what you think.

[hayd]

@one-more-minute note I found/added a few missing methods, which are very obvious once they're grouped by type.

a little overly fine grained as well.

I don't disagree, I just don't have a better solution! I'm happy to stick some of these files together myself if that makes sense...

[MichaelHatherly]

This should be implemented in a similar way to the term method below otherwise only the first line of the block will have > added. See below:

julia> a = """
       > a
       > 
       > * 1
       > * 2
       >
       > ```
       > aaa
       > ```
       """
"> a\n> \n> * 1\n> * 2\n>\n> ```\n> aaa\n> ```\n"

julia> a |> Markdown.parse |> Markdown.plain |> print
> a

  * 1
  * 2

```
aaa
```

[MichaelHatherly]

This reorganisation makes it easier to navigate I think, though it might be a good idea to expand the tests prior to doing this to make sure we don't cause any regressions due to shuffling everything around.

(@hayd, some notes inline regarding PR #12130).

[hayd]

It would be nice to confirm, with a test, whether we've hit all of the inline methods or all of the block methods for each type. I agree with your comments from the other PR.

Not sure how much expanding the tests will be sufficient. I recommend just trying out this branch on some complex markdown... :/

This re-org makes it much easier to review this for each type/file... the fact that this passes the tests (in the sense that every parsing function is still defined) as well as me looking carefully at each file (checking it defines each of the inline/block render methods), gives me quite a bit of confidence here.

[hayd]

Now we're post 0.4 is this worth revisiting (rebasing)? I think we're agreed than having all the methods for each type in the same file would make code clearer BUT having files just including a type and their methods (the current state of this PR) is too finely-grained... so I'm not sure what the best compromise.

[hayd]

Perhaps a compromise is to remove some of the directory structure for flavors (of CommonMark, IJulia, Github) will make this cleaner (and less nested than before)... Even if the filesizes remained small (one for each type + parsing + rendering). I'm not sure what the purpose of supporting multiple @flavors was (this was brought up elsewhere recently), but I don't think the current directory structure adds value for it.

So it would be:

Markdown/
  render/
  parse/
  blockquote.jl
  bold.jl
  ...
  Markdown.jl

[jakebolewski]

I don't think this is necessary.

[hayd]

Even for the autolinks code (with no additional rendering) I needed to modify 6 files, for a more usual type e.g. horizontal rule it's 8 (#10483). And it's really tricky to spot missing methods.

This refactor is not necessary but it would make reading/editing/maintaining significantly easier.

[tkelman]

Your proposal of dropping the final flavor level of hierarchy sounds reasonable to me, though I haven't been working on this code.

[JeffBezanson]

Has been moved to stdlib, so less necessary and would need to be redone.