Better YAML front matter parsing in commonmark-ext-yaml-front-matter - #445
Better YAML front matter parsing in commonmark-ext-yaml-front-matter#445zyxist wants to merge 2 commits into
Conversation
…ext-yaml-front-matter
robinst
left a comment
robinst
left a comment
There was a problem hiding this comment.
Thanks for working on this! I have a few things I'd like to see changed but the direction is good.
| Map<String, List<String>> frontMatter = YamlFrontMatterVisitor.readData(document); | ||
| ``` | ||
|
|
||
| Alternatively, you can use initialize the extension with `YamlContentExtractor` that saves the YAML front matter content |
|
|
||
| import org.commonmark.node.CustomNode; | ||
|
|
||
| public class YamlFrontMatterContent extends CustomNode { |
There was a problem hiding this comment.
So the existing one is called YamlFrontMatterNode. I'm not sure if just "Content" is enough to distinguish the two. What do you think about naming this one YamlFrontMatterRawContent (and the method getLiteral() (like what code blocks call their content)? Also, can you add some Javadoc here and for YamlFrontMatterNode?
|
|
||
| import org.commonmark.parser.block.BlockContinue; | ||
|
|
||
| public interface YamlFrontMatterExtractor { |
There was a problem hiding this comment.
Hmm not sure about calling this an "extractor", it feels like what the visitor does sounds more like extracting. We already have a good word for turning some input into nodes, which is a parser. Should we just call this FrontMatterParser?
For the name of the implementations I have similar concerns (them not being specific enough). With the above change, they would be:
BasicYamlParserRawContentParser
(Note that the raw content parser could also be used for e.g. TOML or other syntax of front matter.)
There was a problem hiding this comment.
You're right, I struggled to find the right naming here. I initially rejected "parser", because I had a very similar name YamlFrontMatterBlockParser next to it, but the term is perfect here. I will take a look :)
| public interface YamlFrontMatterExtractor { | ||
| void onNextLine(YamlFrontMatterBlock block, CharSequence line); | ||
|
|
||
| BlockContinue onBlockEnd(YamlFrontMatterBlock block); |
There was a problem hiding this comment.
I would remove the return type here, as anything else than BlockContinue.finished() doesn't make sense.
There was a problem hiding this comment.
Actually, it allows the parser to handle the following use case described here, if someone needs it:
But this is a minor issue with easy workaround, so I can either simplify the interface or document the purpose in the javadoc.
| import org.commonmark.parser.block.BlockContinue; | ||
|
|
||
| public interface YamlFrontMatterExtractor { | ||
| void onNextLine(YamlFrontMatterBlock block, CharSequence line); |
There was a problem hiding this comment.
Can you change the line to SourceLine instead (which is what parserState.getLine() returns)?
| /** | ||
| * Reads the YAML Front Matter metadata as a string, if the Markdown | ||
| * document has the YAML Front Matter and the extension uses | ||
| * {@link YamlContentExtractor} (default). |
There was a problem hiding this comment.
| * {@link YamlContentExtractor} (default). | |
| * {@link YamlContentExtractor}. |
2 participants
Last year, I reported the issue #391 - I found some time to turn it into the contribution, based on the outcome of the discussion. I will be happy to discuss the approach and apply any improvements, if necessary.
Summary
The PR splits the YAML front matter processing into two parts:
YamlFrontMatterExtractor.I moved the existing built-in parser of YAML subset into one of the two extractors:
YamlDataExtractor. The other extractor isYamlContentExtractorthat extracts the front matter as a string for further processing with other tools.Compatibility
The changes are backward-compatible.
YamlDataExtractoris the default extractor, and one must explicitly initialize the extension withYamlContentExtractorto get the string content. The visitor and Markdown renderer support both modes.Naming
For the existing parser, I strived to keep consistent naming to help understand the connection:
YamlFrontMatterVisitor.getData()(existing method) =>YamlDataExtractor.