-
-
Notifications
You must be signed in to change notification settings - Fork 149
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
Description Lists Extension #86
Conversation
…e details per item.
I’m incredibly impressed. I’ll take time to review this tomorrow, but I just wanted to leave a comment now to say how amazing this is! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment!
@@ -64,6 +64,7 @@ fn main() { | |||
"tasklist", | |||
"superscript", | |||
"footnotes", | |||
"description-lists", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We add description-lists
as a possible -e
value here …
src/main.rs
Outdated
clap::Arg::with_name("description-lists") | ||
.long("description-lists") | ||
.help("Parse description lists"), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… and add --description-lists
as a separate flag here ...
src/main.rs
Outdated
@@ -125,6 +131,7 @@ fn main() { | |||
ext_superscript: exts.remove("superscript"), | |||
ext_header_ids: matches.value_of("header-ids").map(|s| s.to_string()), | |||
ext_footnotes: matches.is_present("footnotes"), | |||
ext_description_lists: matches.is_present("description-lists"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… and use the flag toggle here, not the exts
set. As a result:
$ cargo run <(echo "a"; echo; echo ": b") --description-lists
Finished dev [unoptimized + debuginfo] target(s) in 0.06s
Running `target/debug/comrak /dev/fd/63 --description-lists`
<dl><dt>
<p>a</p>
</dt>
<dd>
<p>b</p>
</dd>
</dl>
$ cargo run <(echo "a"; echo; echo ": b") -e description-lists
Finished dev [unoptimized + debuginfo] target(s) in 0.06s
Running `target/debug/comrak /dev/fd/63 -e description-lists`
thread 'main' panicked at 'assertion failed: exts.is_empty()', src/main.rs:137:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
$
Ideally description lists should be an -e
option. (And so should footnotes, frankly.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops.. I didn't notice that =/.
Fixed in d7469e7.
Thank you! ❤️ |
Awesome writeup and patch. |
This patch adds support for description lists, as described in #52.
Overview
A description-list is written as a group of terms, each one with a description (details). It is required to put a blank line between terms and descriptions.
For example, the following source:
Is rendered to:
It also supports nested nodes. The following source:
Is rendered to:
Implementation Details
I tried to imitate the support for regular lists in some parts of the implementation, mostly to support nested nodes.
There are four new node variants in
NodeValue
:DescriptionList
DescriptionItem
DescriptionTerm
DescriptionDetails
DescriptionItem
is used only for the parser. It is not emitted in the HTML.To detect a description list:
:
symbol as the first non-space character.DescriptionList
.DescriptionList
node.DescriptionItem / DescriptionTerm / $old-paragraph
, and append it to theDescriptionList
node.DescriptionDetails
node underDescriptionItem
, and use it as the new container.A tricky part of the implementation is that I have to change the
open
field of an existingDescriptionList
. This is required because every new term is created as a paragraph (before detect the:
line with the description), so the previous list is closed. I don't know if this is the proper way to do it.To visualize this:
If we have this source:
The behaviour of the parser is something like:
A
:Paragraph
node, and append it to the top container.:
:Paragraph
, so it is detached.DescriptionList
at the end of the container, so it creates a new one.DescriptionDetails
is new current container to add nodes.B
after:
:Paragraph
and add it to theDescriptionDetails
node.C
:DescriptionList
and add the text as a newParagraph
.:
:Paragraph
, so it is detached.DescriptionList
as the last child.open
.DescriptionItem
node to the current list.The requirement of the blank line between terms and descriptions is to keep the implementation simpler. Many parsers supports a syntax like:
But it was not clear to me how to support this. I guess that this is something that can be added later.
Changes in the CLI
The CLI supports the flag
description-lists
to enable the extension.S-Expressions in the examples
An example program in
examples/s-expr.rs
can be used to print the AST as S-expressions.$ cargo run --example s-expr file1.md file2.md ... $ cat file.md | cargo run --example s-expr
For example, the following source:
Generates this output:
I added it to the patch because it was useful to debug the AST, and it may be useful for other people. If you think that it should not be included, I will remove it.
Closes #52