Skip to content
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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

strub
Copy link

@strub strub commented Dec 10, 2020

See issue #40.

@rgrinberg
Copy link
Owner

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.

@strub
Copy link
Author

strub commented Dec 10, 2020

Or we make yaml an optional dependency?

@rgrinberg
Copy link
Owner

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.

@strub
Copy link
Author

strub commented Dec 10, 2020

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.

@rgrinberg
Copy link
Owner

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.

@gasche
Copy link
Collaborator

gasche commented Dec 25, 2020

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
Copy link
Collaborator

gasche commented Dec 25, 2020

@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) :-)

@strub
Copy link
Author

strub commented Dec 25, 2020

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

@rgrinberg
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants