Make YAML metadata parsing function configurable

[Shados]

lyaml is a drop-in replacement for lcmark's usage. Both modules are bindings to the same C library (LibYAML), but:

• lyaml is actually maintained.
• lyaml is a binding to the system LibYAML, instead of an ancient vendored copy.

(You should bump the rockspec revision if you do merge this, I guess?)

[jgm]

Since it doesn't include a vendored copy, I assume that means that lcmark wouldn't work unless the dynamic libYAML library is installed in the system? That might be a disadvantage for some users. What about using a pure lua library like lua-yaml?

[Shados]

Yes, it would mean that, as luarocks cannot install system dependencies (although it will correctly report build failures due to lacking system deps, if they're specified properly in the rockspec).

As the translation of YAML data into Lua tables is pretty straightforward and that's all you're using the library for, I suspect lua-yaml or pretty much any other Lua YAML library will work. Unfortunately, luarocks doesn't really have a way of specifying alternates or optional dependencies, so you can't offer a choice directly.

That said, users can force luarocks to act as if they already have some package externally installed (via the rocks_provided config option), so it is possible to effectively stub out whatever yaml library you require in the rockspec.

If you then defaulted to lua-yaml (or whatever), and provided a module function that configures the function to use when loading yaml metadata, that would make the choice of library effectively up to the user (e.g. function lcmark.set_yaml_loader(yaml_load_fn)).

[jgm]

Alexei Robyn notifications@github.com writes:

If you then defaulted to lua-yaml (or whatever), and provided a module function that configures the function to use when loading yaml metadata, that would make the choice of library effectively up to the user (e.g. function lcmark.set_yaml_loader(yaml_load_fn)).

I'd be fine with this sort of approach.

[Shados]

OK. Later today, I'll update the PR to:

• Use a pure-lua YAML library by default
• Allow the YAML loader function to be configured
• Add some brief documentation on doing that

[Shados]

Well, there aren't really any good pure-Lua YAML libraries on luarocks, so I've left the rockspec dependency as yaml, but the other changes have now been made.

[craigbarnes]

Well, there aren't really any good pure-Lua YAML libraries on luarocks

There aren't any pure-Lua YAML parsers on LuaRocks at all. lua-yaml isn't a rigorous parser suitable for arbitrary YAML input. Using it would be a massive downgrade in correctness compared to libyaml bindings.

[Shados]

@craigbarnes yes, that's why I changed the commit to leave it as-is. With the restriction of not having any system dependencies, the rather old vendored copy of libyaml in the yaml rock is the best option. Although I forgot to update the commit message, so I'll fix that now...

[jgm]

Presumably version should bump too to reflect API change. If we want to keep the first number in the version tied to the commonmark version, maybe to 0.29.1?

[Shados]

I have no strong feelings on it. It does seem pretty clear to me that a version can either tell the user what cmark-lua/cmark/commonmark version it is compatible against, or it can convey information about the API and/or fixes, not really both. Although if you did want to do both, i guess you could smush two version numbers together :P.

[jgm]

The numbering system is the one we've been using for cmark and commonmark.js. It's probably not ideal, but given that the others are using this, we should probably do the same here. The thought was that when the spec got to 1.0 we'd switch to a more conventional version number scheme.

[RiskoZoSlovenska]

I'd like to bump this. yaml (the YAML library lcmark currently uses) is very old (libyaml 0.1.3, as opposed to the current 0.2.5) and is full of issues such as segfaulting when parsing nulls, incorrectly parsing null as a string, not parsing dates correctly, etc. Additionally, it doesn't support local options and has no way to represent null values. At this point, switching to something like lua-yaml might actually even be beneficial.

Additionally, it may be worthwhile to consider removing yaml from the list of luarocks dependencies and having users install it manually. The last time I checked, yaml and lyaml conflict with each other if they're installed simultaneously.

[jgm]

I'm most tempted to switch to the pure Lua lua-yaml. Usually the YAML used in metadata is quite simple, and if the parser is correct for simple YAML, that should be sufficient. However, I'm not really up on lua-yaml's specific shortcomings. If we went this way, adding an option to use lyaml instead would also be nice.

[RiskoZoSlovenska]

However, I'm not really up on lua-yaml's specific shortcomings.

I just checked lua-yaml's Issues and PRs page, and I retract my statement about it possibly being more robust than yaml - it looks to be in an even worse state.

Looking at the available Lua YAML libraries/bindings, I feel like letting users use lyaml (or, alternatively, any parser of their choice) would definitely be a huge benefit.

All being said, this PR is a bit old now and only adds global configuration; I think I'll PR an alternate implementation sometime tomorrow.

[RiskoZoSlovenska]

Now that https://github.com/jgm/lcmark/pull/11 is merged, I'm pretty sure this can be closed.

Also, could you please bump the version/revision and publish to luarocks?

[jgm]

I've published 0.30.2-1 (and this also gives you updated cmark).