Make definitions block-level #257
Conversation
This allows multiple paragraphs in a definition. See examples at https://michelf.ca/projects/php-markdown/extra/#def-list
|
Looks like we'll also need to run ocamlformat, btw. |
That would be great - thanks! Your suggestions all sound sensible to me. |
The lists want to... be with the lists
tests/def_list.md
Outdated
| <p>Note that, currently, subsequent paragraphs cannot lazy wrap.</p> | ||
| </dd> | ||
| </dl> | ||
| <p>For example, this line will be part of a new paragraph outside | ||
| of the definition list entirely.</p> |
There was a problem hiding this comment.
@talex5 does this seem like the right behavior to you? I'm not exactly sure what ought to happen in this case. I can report that in pandoc, this markdown
Term 1
: A first paragraph that is
lazily wrapped
A second paragrap that
is lazily wrapped
is rendered as
<dl>
<dt>Term 1</dt>
<dd><p>A first paragraph that is lazily wrapped</p>
<p>A second paragrap that is lazily wrapped</p>
</dd>
</dl>But they use 4 spaces to indicate the start of the second paragraph.
| | Rdef_list deflist, _ when Parser.indent s >= deflist.indent -> | ||
| let s = Sub.offset deflist.indent s in | ||
| let state = process deflist.state s in | ||
| { blocks; next = Rdef_list { deflist with state } } |
There was a problem hiding this comment.
I guess this should set empty_line_seen = false to fix the problem with lazy wrapping you mention below.
There was a problem hiding this comment.
This does address the lazy wrapping, thanks for the tip! Unfortunately it also breaks the nested def-list parsing. I'll continue considering it, but if an immediate solution occurs to you, let me know ;)
There was a problem hiding this comment.
Reordering the clauses such that the one that checks for the indentation level comes first solves the problem with the nested def-list test. The only remaining problem on def_list-003 test then are missing <p>s in the generated html, meaning that the list is being parsed as Tight.
| @@ -147,10 +181,34 @@ module Pre = struct | |||
| { blocks | |||
| ; next = Rfenced_code (ind, num, q, info, Sub.to_string s :: lines, a) | |||
| } | |||
| | Rdef_list (term, d :: defs), Lparagraph -> | |||
| | ( Rdef_list ({ empty_line_seen = false; _ } as deflist) | |||
There was a problem hiding this comment.
Thinking about this a bit more, I don't think we need the empty_line_seen = false check here.
There was a problem hiding this comment.
Changing this (either by removing the match on the empty_line_seen or removing the case all together) leads to failures around the This is not a correct definition list line. This to will need some considering. The moral of the story seems to me that the handwritten parsing logic here is quite touchy and tough to reason about...
There was a problem hiding this comment.
https://github.com/jgm/commonmark-hs/blob/master/commonmark-extensions/test/definition_lists.md ("Multiple definitions, loose") gives this example:
apple
: red fruit
: computer company
orange
: orange fruit
: telecom company
.
<dl>
<dt>apple</dt>
<dd>
<p>red fruit</p>
</dd>
<dd>
<p>computer company</p>
</dd>
<dt>orange</dt>
<dd>
<p>orange fruit</p>
</dd>
<dd>
<p>telecom company</p>
</dd>
</dl>
So perhaps the This is not a correct definition list test is itself wrong?
There was a problem hiding this comment.
Ah, thanks for the reference! I'm happy with deferring to commonmark on these things. I'll spend some time comparing their test suite with ours later this week or over the weekend.
There was a problem hiding this comment.
Ah, duh. Now that I look at the test case, since you've introduced the loose spacing, of course that test is now incorrect! My bad for not giving it sufficient thought before.
There was a problem hiding this comment.
So, if we remove this check, then it fixes the loose format for multiple definitions, but it breaks the nesting. Which I think means that, even if we were OK with the leaving the lazy line wrapping broken for multi-paragraph definitions, we're still a bit stuck on this point.
I'll investigate over the weekend, and try to get my head around properly around the parsing function. But just an FYI to explains why I'm not merging this straight away.
Should make it easier to test individual properties and features of the def_list parsing, and provide better documentation.
I just converted my blog from Octopress to omd, and I needed support for definition lists with multiple paragraphs.
Not completely sure what I'm doing here, but this changed got it working it for me, and should go some way towards #221.