Metadata Extension #24

chiwanpark · 2015-10-09T13:57:28Z

This pull request contains following changes:

Add a maven submodule called commonmark-ext-metadata
Add an extension which enables metadata feature
Add metadata documentation into README.md

The metadata syntax is inspired by YAML format. But this extension supports only few features of YAML because of avoiding other YAML dependency and implementing easily.

This is a first pull request which I send to atlassian open source. So there may be many problems to merge this. Please feel free to leave comments. ;)

robinst · 2015-10-11T02:57:50Z

README.md

Replace "This extension only supports limited features" with "This extension only supports a subset of YAML syntax"? And replace "The example is following:" with "Here's an example of what's supported:" to make it clear that this is it.

Also, is there some kind of specification for this header that we can link to?

robinst · 2015-10-11T03:17:03Z

Hey! Very good work, I only have some minor comments, see above. Some more comments:

Can metadata appear in the middle of the document, or does it have to be at the beginning? In case it has to be at the beginning, maybe we should check for that in tryStart?
Please add the extension to SpecIntegrationTest, so that we know we don't break anything in core with this extension.
Do you feel that this level of support for YAML covers almost all- use cases of document in the wild?

chiwanpark · 2015-12-02T16:51:01Z

Sorry for late update. I updated my PR and rebased on current master branch. About questions:

Metadata cannot appear in the middle of the document. I already added a test case covering that (metadataInParagraph).
I added metadata extension to SpecIntegrationTest. The integration test passed.
I'm not sure that this extension covers almost all use cases. But this extension provides features which are similar to GFM and python markdown library (https://pythonhosted.org/Markdown/extensions/meta_data.html).

robinst · 2015-12-07T01:06:22Z

...ark-ext-metadata/src/main/java/org/commonmark/ext/metadata/internal/MetadataBlockParser.java

This won't work. tryStart only gets one line of the document. I think checking that we're at the beginning of the document and that we have a boundary marker is enough.

Also, please add a test for the following input:

--- test

That currently fails because there's an infinite loop in tryStart.

I had another look, and the fact that this is working is actually a bug in the Substring class, it doesn't do the necessary bounds checking. I'm going to fix these bugs on master, which will mean that the above will stop working. But it wouldn't work when reading the input from a file anyway (only one line at a time).

You'll need to merge master and adjust the implementation. Thanks for your patience :).

Ok, pushed to master. Could you merge master into your branch and adapt?

If we check only one line to decide whether metadata block starts or not, there are some collisions with CommonMark spec like following case:

--- test ----

From original CommonMark spec, rendering result should be <hr />\n<h2>test</h2>\n. But because this extension consider --- as start of metadata block, the actual result will be empty. Is there any method to use multi-line input for detecting start of the block?

There isn't currently. If the first line is a normal paragraph line, then there's support for replacing it on the second line with a block. That is used e.g. for headlines, where only the second line (with the -) shows that it's a headline. But in the above case, the first line is parsed as a horizontal rule right away, not a paragraph.

Do you know how e.g. the Python implementation handles this?

Are there more collisions? If we can't find a way, we can exclude the examples in SpecIntegrationTest. I'm not sure it's a problem in practice, because it only affects the very first line. And if a user encounters this case, they can work around it by adding an empty line before it or an empty metadata block using two ... lines.

From my investigation, the Python implementation (https://github.com/waylan/Python-Markdown/blob/master/markdown/extensions/meta.py) receives all unparsed-markdown string and check whole lines to decide whether metadata block starts or not.

If we check block boundary using only a line, example 14 and 16 in Setext headers of SpecIntegrationTest fail.

By the way, the Python implementation and github markdown implementation allow only --- in first line to begin a metadata block. I would like to apply this rule in this extension.

Thanks for investigating!

Looks like the Python library implements it as a pre-processor that removes the metadata block from the input. Then later the rest of the document is parsed. I'm a bit hesitant to explore that direction because it makes source positions more complicated. Also, that doesn't really work nicely when parsing from a Reader instead of a String.

So let's go with keeping it simple for now, and if the first line in the document happens to be a ---, we interpret it as a metadata block. We could do some more checking in tryContinue and stop the block if the first line of content doesn't look like metadata content.

That means we'll need to override example 14 and 16 in SpecIntegrationTest, see getOverriddenExamples.

Also, +1 for only allowing ---.

chiwanpark · 2015-12-07T13:24:31Z

I addressed your comments @robinst. :)

Otherwise it's possible to access more of the underlying string than the substring covers. Discovered in #24. Not sure if Substring is really worth it at this point.

chiwanpark · 2016-01-15T03:13:33Z

Hi @robinst, sorry for late update. I added a block verification logic to tryContinue method and overridden examples.

robinst · 2016-01-16T02:10:16Z

...ark-ext-metadata/src/main/java/org/commonmark/ext/metadata/internal/MetadataBlockParser.java

Is this intentional, allowing ... to end the block but not to start it?

Replying to myself here, but looks like other implementations also do that: https://github.com/jgm/lcmark/#yaml-metadata

So disregard my comment :).

derari · 2016-02-19T12:28:34Z

Just a quick side note: Wouldn't calling this extension "YAML front matter" (because that's what it is, even if not all YAML is supported) be more descriptive than just calling it "Metadata"? There is probably more than one way one might encounter metadata when handling Markdown, so I think it might be helpful for user if it was clear from the extension's name where and what kind of metadata it supports.

robinst · 2016-02-23T01:26:07Z

@derari: I think that's a good point. @chiwanpark, when you come around to updating this PR, could you make that change? Other than that, we're really close, and I'd love to see this included in the next release :).

chiwanpark · 2016-02-23T01:42:44Z

I'm really sorry for delaying update @robinst. I'll update this PR including @derari's suggestion in 3 days.

About a name of maven module, is commonmark-ext-yaml-frontmatter reasonable? I found that a similar implementation in Jekyll is called frontmatter also (http://jekyllrb.com/docs/frontmatter/).

robinst · 2016-02-23T02:49:33Z

Cool! I'd add another hyphen, commonmark-ext-yaml-front-matter, because it's multiple words.

- Move `literal` into `parseInlines` method - Fix a bug in `parseInlines` for incomplete block - Simplify construction of `MetadataNode` - Add a test case for incomplete metadata block

…ethod

- Rename the extension from `Metadata` to `YAMLFrontMatter`

chiwanpark · 2016-02-24T13:07:10Z

Hi all, I updated this PR addressing your opinions. Please review this. :-)

robinst · 2016-02-24T23:53:38Z

...onmark-ext-yaml-front-matter/src/main/java/org/commonmark/ext/yaml/YAMLFrontMatterBlock.java

@@ -0,0 +1,6 @@
+package org.commonmark.ext.yaml;


Can you change the packages to org.commonmark.ext.front.matter instead? (Seems more future proof, apparently there's other front matter formats, e.g. JSON.)

chiwanpark · 2016-02-25T11:11:44Z

Thanks for review @robinst. I just addressed your comments.

robinst · 2016-02-26T06:30:37Z

...xt-yaml-front-matter/src/main/java/org/commonmark/ext/front/matter/YAMLFrontMatterBlock.java

+
+import org.commonmark.node.CustomBlock;
+
+public class YamlFrontMatterBlock extends CustomBlock {


Looks like the file names aren't changed, this file is still named YAMLFrontMatterBlock. That's why the Travis build fails. Are you on a case-insensitive file system? Check out this Stackoverflow question for how to work around that.

chiwanpark · 2016-02-26T09:08:58Z

Hi @robinst, I updated the all filenames. Thanks for your guide.

YAML front matter metadata extension

robinst · 2016-02-26T13:16:02Z

Finally merged 😄. Nice work Chiwan!

chiwanpark · 2016-02-27T04:58:29Z

Thanks @robinst and @derari!

robinst · 2016-04-22T13:50:02Z

Released in 0.5.0 now 🎉

robinst reviewed Oct 11, 2015
View reviewed changes

chiwanpark force-pushed the metadata-extension branch 2 times, most recently from dad0ba8 to a167118 Compare December 2, 2015 16:44

robinst reviewed Dec 7, 2015
View reviewed changes

robinst added a commit that referenced this pull request Dec 8, 2015

Add more bounds checks to Substring

838c98e

Otherwise it's possible to access more of the underlying string than the substring covers. Discovered in #24. Not sure if Substring is really worth it at this point.

chiwanpark force-pushed the metadata-extension branch 2 times, most recently from 156ab60 to 4ede3f9 Compare January 15, 2016 03:13

robinst reviewed Jan 16, 2016
View reviewed changes

chiwanpark added 4 commits February 24, 2016 20:38

Add Metadata extension

b958ebf

Address comments

d1aa3bf

Address comments

900f5f7

- Move `literal` into `parseInlines` method - Fix a bug in `parseInlines` for incomplete block - Simplify construction of `MetadataNode` - Add a test case for incomplete metadata block

Revert checking multilines in tryStart method

e04ccdc

chiwanpark added 4 commits February 24, 2016 20:38

Update version

a30c087

Add overridden examples and a block decision logic in tryContinue m…

d607daf

…ethod

Recover test

628800c

Refactor YAML front matter extension

4e83c35

- Rename the extension from `Metadata` to `YAMLFrontMatter`

chiwanpark force-pushed the metadata-extension branch from 16abe6f to 4e83c35 Compare February 24, 2016 13:03

Update README.md

2354c89

robinst reviewed Feb 24, 2016
View reviewed changes

Rename base package and classes

7c3202a

robinst reviewed Feb 26, 2016
View reviewed changes

chiwanpark added 2 commits February 26, 2016 18:06

Rename files

a7918f7

Rename class name in javadoc

e2d2228

robinst added a commit that referenced this pull request Feb 26, 2016

Merge pull request #24 from chiwanpark/metadata-extension

77ddc83

YAML front matter metadata extension

robinst merged commit 77ddc83 into commonmark:master Feb 26, 2016


		import org.commonmark.node.CustomBlock;

		public class YamlFrontMatterBlock extends CustomBlock {

Metadata Extension #24

Metadata Extension #24

Uh oh!

Conversation

chiwanpark commented Oct 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robinst commented Oct 11, 2015

Uh oh!

chiwanpark commented Dec 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chiwanpark commented Dec 7, 2015

Uh oh!

chiwanpark commented Jan 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derari commented Feb 19, 2016

Uh oh!

robinst commented Feb 23, 2016

Uh oh!

chiwanpark commented Feb 23, 2016

Uh oh!

robinst commented Feb 23, 2016

Uh oh!

chiwanpark commented Feb 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chiwanpark commented Feb 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chiwanpark commented Feb 26, 2016

Uh oh!

robinst commented Feb 26, 2016

Uh oh!

chiwanpark commented Feb 27, 2016

Uh oh!

robinst commented Apr 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants