-
Notifications
You must be signed in to change notification settings - Fork 25
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
Hogan-style template inheritance #58
Conversation
@@ -0,0 +1 @@ | |||
{"overview":"Parent tags are used to expand an external template into the current template,\nwith optional parameters delimited by block tags.\n\nThese tags' content MUST be a non-whitespace character sequence NOT containing\nthe current closing delimiter; each Parent tag MUST be followed by an End\nSection tag with the same content within the matching parent tag.\n\nBlock tags are used inside of parent tags to assign data onto the context stack \nprior to rendering the parent template. Outside of parent tags, block tags are\nused to indicate where value set in the parent tag should be placed. If no value\nis set then the content in between the block tags, if any, is rendered.\n","tests":[{"name":"Default","desc":"Default content should be rendered if the block isn't overridden","data":{},"template":"{{$title}}Default title{{/title}}\n","expected":"Default title\n"},{"name":"Variable","desc":"Default content renders variables","data":{"bar":"baz"},"template":"{{$foo}}default {{bar}} content{{/foo}}\n","expected":"default baz content\n"},{"name":"Triple Mustache","desc":"Default content renders triple mustache variables","data":{"bar":"<baz>"},"template":"{{$foo}}default {{{bar}}} content{{/foo}}\n","expected":"default <baz> content\n"},{"name":"Sections","desc":"Default content renders sections","data":{"bar":{"baz":"qux"}},"template":"{{$foo}}default {{#bar}}{{baz}}{{/bar}} content{{/foo}}\n","expected":"default qux content\n"},{"name":"Negative Sections","desc":"Default content renders negative sections","data":{"baz":"three"},"template":"{{$foo}}default {{^bar}}{{baz}}{{/bar}} content{{/foo}}\n","expected":"default three content\n"},{"name":"Mustache Injection","desc":"Mustache injection in default content","data":{"bar":{"baz":"{{qux}}"}},"template":"{{$foo}}default {{#bar}}{{baz}}{{/bar}} content{{/foo}}\n","expected":"default {{qux}} content\n"},{"name":"Inherit","desc":"Default content rendered inside included templates","data":{},"template":"{{<include}}{{/include}}\n","partials":{"include":"{{$foo}}default content{{/foo}}"},"expected":"default content"},{"name":"Overridden content","desc":"Overridden content","data":{},"template":"{{<super}}{{$title}}sub template title{{/title}}{{/super}}","partials":{"super":"...{{$title}}Default title{{/title}}..."},"expected":"...sub template title..."},{"name":"Data does not override block","desc":"Provided data does not override data passed into parent","data":{"var":"var in data"},"template":"{{<include}}{{$var}}var in template{{/var}}{{/include}}","partials":{"include":"{{$var}}var in include{{/var}}"},"expected":"var in template"},{"name":"Data does not override block default","desc":"Provided data does not override default value of block","data":{"var":"var in data"},"template":"{{<include}}{{/include}}","partials":{"include":"{{$var}}var in include{{/var}}"},"expected":"var in include"},{"name":"Overridden partial","desc":"Overridden partial","data":{},"template":"test {{<partial}}{{$stuff}}override{{/stuff}}{{/partial}}","partials":{"partial":"{{$stuff}}...{{/stuff}}"},"expected":"test override"},{"name":"Two overridden partials","desc":"Two overridden partials with different content","data":{},"template":"test {{<partial}}{{$stuff}}override1{{/stuff}}{{/partial}} {{<partial}}{{$stuff}}override2{{/stuff}}{{/partial}}\n","partials":{"partial":"|{{$stuff}}...{{/stuff}}{{$default}} default{{/default}}|"},"expected":"test |override1 default| |override2 default|\n"},{"name":"Override partial with newlines","desc":"Override partial with newlines","data":{},"template":"{{<partial}}{{$ballmer}}\npeaked\n\n:(\n{{/ballmer}}{{/partial}}","partials":{"partial":"{{$ballmer}}peaking{{/ballmer}}"},"expected":"peaked\n\n:(\n"},{"name":"Inherit indentation","desc":"Override one substitution but not the other","data":{},"template":"{{<partial}}{{$stuff2}}override two{{/stuff2}}{{/partial}}","partials":{"partial":"{{$stuff}}new default one{{/stuff}}, {{$stuff2}}new default two{{/stuff2}}"},"expected":"new default one, override two"},{"name":"Super template","desc":"Super templates behave identically to partials when called with no parameters","data":{},"template":"{{>include}}|{{<include}}{{/include}}","partials":{"include":"{{$foo}}default content{{/foo}}"},"expected":"default content|default content"},{"name":"Recursion","desc":"Recursion in inherited templates","data":{},"template":"{{<include}}{{$foo}}override{{/foo}}{{/include}}","partials":{"include":"{{$foo}}default content{{/foo}} {{$bar}}{{<include2}}{{/include2}}{{/bar}}","include2":"{{$foo}}include2 default content{{/foo}} {{<include}}{{$bar}}don't recurse{{/bar}}{{/include}}"},"expected":"override override override don't recurse"},{"name":"Multi-level inheritance","desc":"Top-level substitutions take precedence in multi-level inheritance","data":{},"template":"{{<parent}}{{$a}}c{{/a}}{{/parent}}","partials":{"parent":"{{<older}}{{$a}}p{{/a}}{{/older}}","older":"{{<grandParent}}{{$a}}o{{/a}}{{/grandParent}}","grandParent":"{{$a}}g{{/a}}"},"expected":"c"},{"name":"Multi-level inheritance, no sub child","desc":"Top-level substitutions take precedence in multi-level inheritance","data":{},"template":"{{<parent}}{{/parent}}","partials":{"parent":"{{<older}}{{$a}}p{{/a}}{{/older}}","older":"{{<grandParent}}{{$a}}o{{/a}}{{/grandParent}}","grandParent":"{{$a}}g{{/a}}"},"expected":"p"},{"name":"Text inside super","desc":"Ignores text inside super templates, but does parse $ tags","data":{},"template":"{{<include}} asdfasd {{$foo}}hmm{{/foo}} asdfasdfasdf {{/include}}","partials":{"include":"{{$foo}}default content{{/foo}}"},"expected":"hmm"},{"name":"Text inside super","desc":"Allows text inside a super tag, but ignores it","data":{},"template":"{{<include}} asdfasd asdfasdfasdf {{/include}}","partials":{"include":"{{$foo}}default content{{/foo}}"},"expected":"default content"}],"__ATTN__":"Do not edit this file; changes belong in the appropriate YAML file."} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pretty-print this file perhaps? Or is the minification required for the test?
@@ -0,0 +1,192 @@ | |||
overview: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this yaml file added for the purpose of documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is also related to your other question on .json
minification). Specs in the official repository are written in YAML format as the reference, and then programmatically rendered into JSON to be usable for people outside the YAML cult.
The existing spec files in the specs/
directory of ocaml-mustache exactly follow this format: both versions were copied over, the JSON version is used by the tool and the YAML version is read by humans when a test fails. The import I did here follows the same approach.
I think that it is okay to keep the current (non-ideal) workflow. In any case, we should keep the YAML files around, because they are the reference format, so we want people to be able to "diff" them against the current version of the spec to see if we missed some changes. (The spec repository currently appears to be unmaintained, but oh well.)
We could probably add some tooling of our side to re-render the JSON files from the YAML file with human-friendly pretty-printing. (One fun aspect of it is that the JSON specs from the upstream repo are currently out of date, so doing this refresh would in fact change slightly the tests we are running. Who knows, they may even catch a mistake and start failing.) I would propose to wait until there is some yaml
library used here (possibly from #45) to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. We should note this in a ticket then. Using a yaml library for the tests sounds fine as well.
section ["foo"] (section ["bar"] (raw "Middle\n")); | ||
raw "End\n"; | ||
] | ||
, [ ( `O [ "foo" , `O []; "bar", `O [] ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cram test would make it easier to demonstrate the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a difficulty in terms of test organization, though: if we want to split the binary as a separate package, I probably shouldn't write a cram test using the binary in the library tests, as this would introduce a suspicious circular-like dependency (lib{test} -> bin -> lib). (For now the cram tests in bin/test
are more like integration tests for the binary behavior, rather than arbitrary spec tests.)
I could write a cram test using the library only. For now I went the easy way of just using 0-indented string literals with real newlines instead of \n
escapes, to make the current test easier to read and understand.
@@ -112,7 +112,7 @@ Mismatch between section-start and section-end: | |||
$ mustache foo.json $PROBLEM | |||
Template parse error: | |||
File "wrong-nesting.mustache", lines 1-2, characters 41-0: | |||
Section mismatch: {{#foo}} is closed by {{/bar}}. | |||
Open/close tag mismatch: {{# foo }} is closed by {{/ bar }}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this style not apply to variables? E.g. {{$ foo }}..{{/ foo}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, I just added test cases for {{$foo}}
and {{<foo}}
. Feel free to recommend improvements to the wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, what I meant was that pp_partial_param
isn't consistent with this printing style. It's still printing {{$foo}}
instead of {{$ foo }}
.
Thanks @rgrinberg for the review! (I hope that I am not disturbing you in some sort of well-deserved vacation with my PRs. Please feel free to take a few days to handle them.) A point worth noting is that this PR strengthens the OCaml version requirement from 4.06 to 4.08. There is no very strong reason for this, I just happened to have a use for (Debian stable is currently at 4.05, if I read correctly.) |
b62280b
to
9abc3cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use 4.06 or 4.07 so I have no problem with dropping support.
9abc3cd
to
9cf896f
Compare
This is interesting, I think this was one of the missing feature that made mustache not-so-suitable for https://discuss.ocaml.org/t/choice-of-template-language-for-tyxml/1863 . Maybe I could try again.... |
669fc45
to
9a4feed
Compare
MetaI spent a few days stuck in a hole, trying to get a version of this patch that would:
I have a version that does (3) but not (2): I think it handles whitespace better than other mustache implementations, but obviously this results in different (nicer) outputs on some templates. I think that consistency is very important, so I gave up on this approach for now. (In case people would be curious, which I don't recommend, I saved it here). I pushed a commit that does (1) and (2), but not (3): the code is not much worse than the current implementation of whitespace handling, but it still adds a bit of complexity to that obscure part of the codebase and I'm not proud of it. But I really wanted my output to be indented nicely, so I went with it and included it in the PR. Indentation of template parametersThere are two components that are important for nice indentation of template parameters:
Substracting definition-site indentationConsider: template.mustache:
layout1.mustache:
In this example, what we expect is that When printing a line coming from a parameter, the indentation should be the use-site indentation minus the definition-site indentation of the parameter. (This means that some line sometimes get printed with negative indentation, when the definition-site indentation is higher as in this example. This means that the corresponding number of spaces is skipped at the beginning of the line.) This adds a bit of code to the implementation, but this code is not particular difficult/complex so I guess it is fine. Measuring a parameter indentationIn languages that encourage indentation (typically HTML), a typical definition of a parameter would look like
If you consider that the Instead the present implementation considers that the indentation of a parameter is defined as follows:
(Consistently with the Mustache specification, if the parameter tag is not "standalone" (there are non-whitespace characters before it, then no indentation is counted at all and the output is very ugly. Partials already work that way.) This change is a bit more annoying because it adds a "lookahead" logic (looking into the next line) to the kludge in the lexer that deals with standalone tokens. |
I have also added a Changes entry, and some documentation in the tool manpage. I am satisfied with the current state of the PR and believe it is mergeable, but I'll leave it around for a few days in case people have feedback on the indentation stuff. |
The previous approach to standalone-token handling would peek into a prefix of the line, and decide what to do based on the prefix. This relied on the property that we would have at most one standalone token per line. This is unfortunately not true, consider: ``` Begin. {{#foo}} {{#bar}} Middle. {{/bar}} {{/foo}} End. ``` The new approach processes the whole line at once, failing if it encounters non-whitespace non-standalone token.
The implementation follows the spec proposal: mustache/spec#75 which is supported by at least the following Mustache implementations: - hogan.js - mustache.php - mustache.java - GRMustache (Obj-C) and GRMustache.swift (Swift) - Text::Caml (Perl) - hxmustache (Haxe) - MuttonChop (Swift)
The specs come from the semi-official specification mustache/spec#75
9a4feed
to
9e4daeb
Compare
Thanks! I rebased and plan to merge when the CI comes back. (This feature is kind of giving me the cold feet because it is large and not-as-standard-as-the-other and it's a more invasive code change, but I think it's time to settle down and, as people say, "ship it" -- I do think it would make the library useful for more use-cases, as pointed out by @Drup.) |
This PR implements Hogan-style "template inheritance".
A simple way to think of this feature is that partials behave like functions (expanded at a "callsite"), and template inheritance adds functions that take arguments: at the callsite of a partial, we can pass template fragments as named arguments. For example, running
mustache empty.json caller.mustache
withfunction.mustache:
caller.mustache:
renders as
This template is parametrized over an argument.
Simple partials
{{>foo}}
allow to place the content a template defined elsewhere at a leaf of a template file. Partial-with-arguments{{<foo}}{{$arg}}..{{/arg}}{{/foo}}
allow to place the content of a template around parts of the current template.This feature is specified in a pull-request to the "Mustache specs" repository:
mustache/spec#75
and supported by many Mustache implementations:
I have a use for the feature myself. I don't really like the syntax that was chosen, but I think there is real value in using the same syntax and semantics as other implementations. The proposed implementation was tested against the tests from the proposed specification.
Compatibility
The ASTs of the Mustache implementation have changed: the
Partial
constructor takes an extra parameterparams
(None
for usual partials,Some [...]
for partials with arguments), and there is a newPartial_param
constructor. This breaks compatibility.(Having to change two ASTs at once, for the with-location and the no-location version, proved a bit painful when implementing this feature. I might try to factor the two ASTs using a parametrized type.)
Standalone fun
The tests in the specification revealed a shortcoming of the current "standalone handling" implementation: it does not support cases where several standalone tags are on the same line, for example
{{#foo}}{{#bar}}
. This was not tested in existing specifications, but there is a test in the inheritance specification that checks that{{<foo}}{{$param}}
is handled as a standalone line. To pass this test, I had to change the standalone-handling implementation (I tried several things). The new implementation passes all the previous tests, and the template-inheritance tests.Partial-with-arguments introduce standalone blocks with an open and a close tag (
{{<foo}}...{{/foo}}
instead of just{{>foo}}
). This is difficult for a lexer-postprocessing approach to handle, and several cases are in fact not considered as "standalone lines" when they arguably should:I have an idea of how those cases could be fixed, but this is more subtle code to handle corner cases, and conceptually ugly code. Instead I propose to just forget about these corner cases. There is an easy workaround (to get the expected whitespace behavior in the output), which is to always have
{{<foo}}
and{{/foo}}
on their own lines.