ocaml-mustache icon indicating copy to clipboard operation
ocaml-mustache copied to clipboard

Add support for YAML + option for non-strict mode + change sections behaviour

Open strub opened this issue 4 years ago • 9 comments

See issue #40.

strub avatar Dec 10 '20 17:12 strub

Unfortunately, the yaml library contains C which makes it less portable than we'd like. What about introducing a separate package for the binary that we provide? This binary can definitely introduce the yaml dependency.

rgrinberg avatar Dec 10 '20 20:12 rgrinberg

Or we make yaml an optional dependency?

strub avatar Dec 10 '20 20:12 strub

Optional dependencies are quite problematic and should be avoided as much as possible. We don't want our package to recompile just because the user might have chose to install the yaml packages. This might cause an unexpected build failure that might break the user's switch.

rgrinberg avatar Dec 10 '20 20:12 rgrinberg

Ok, fine. I just want YAML in the CLI, whatever solution you prefer. Do you have in mind an opam package that does that? It is not crystal clear to me how you tell dune to take the installed library and not to (re)build it.

strub avatar Dec 10 '20 22:12 strub

Do you have in mind an opam package that does that? It is not crystal clear to me how you tell dune to take the installed library and not to (re)build it.

I was thinking the following:

  • Introduce a new package in this repository called mustache-cli (dune has no problem with multiple packages per repo). This package would depend on mustache, yaml, and whatever else is needed for the cli
  • Make the mustache binary a part of this new package.
  • The old mustache package will now contain only the library.

rgrinberg avatar Dec 10 '20 22:12 rgrinberg

I think that the last part of the PR ("change sections behaviour") is trying to fix #37, just like #41 and my now-merged #49. This part should not be necessary anymore.

(It's impressive how many different people hit this problem and tried to fix it in different ways! I suspect that your fix may have worked, and it's less invasive than #49, but it's probably better to get rid of the list concatenation on context entry anyway.)

gasche avatar Dec 25 '20 21:12 gasche

@rgrinberg I support your suggestion to split the binary in a different package with other dependencies, especially if you want the core library to remain Mirage-friendly. As the master of ceremony, I would encourage you to do the split (when you have time) :-)

gasche avatar Dec 25 '20 21:12 gasche

Ok, I'll remove that commit. (And yes, I took the shortest path to something working :) )

strub avatar Dec 25 '20 21:12 strub

@strub could you rebase this PR please? I'll do the package split myself.

rgrinberg avatar Dec 28 '20 01:12 rgrinberg