Clarify context stacks #114
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
After preparing this PR I realized that fairly old PRs lying around unanswered are trying to cover the same hole in the specification: #31 from January 2012, and #95 from October 2015. The new tests that I am proposing supersede #31, but not #95, so I just included the commit of #95 in the current PR. Who is maintaining this repository these days? (cc @locks, who is the last to have a merged a Pull Request.) These changes should be uncontroversial as they are not adding new features, just improving the testing coverage for existing features, and it is unfortunate that they were delayed for so long. |
gasche
added a commit
to gasche/ocaml-mustache
that referenced
this pull request
Dec 24, 2020
There was a bug in a corner case of the variable-lookup semantics,
which is important because it corresponds to a fairly natural "if
variable exists" pattern (in non-strict-mode). Consider the following
example when `var` is mapped to a scalar value in the context:
{{#var}} show the value {{var}} of the defined variable var {{/var}}
The current ocaml-mustache implementation will fail: when entiring the
`var` section, it would define the lookup context as the scalar value
itself, and any name lookup within the section would fail.
Instead the Mustache documentation and specification require that if
lookup in the current context fails (here the current context defines
no name), then "parent contexts" should be looked up; in particular,
the context before the section was entered, in which the lookup
succeeds.
> A {{name}} tag in a basic template will try to find the name key in
> the current context. If there is no name key, the parent contexts
> will be checked recursively. If the top context is reached and the
> name key is still not found, nothing will be rendered.
The present PR implements this "recursive lookup" semantics. Note that
falling back to parent contexts is *not* allowed in the middle of
resolving a dotted name (we find the right context for the first part
of the name, and then all subsequent lookups happen in
this context). This is explicitated in the Mustache specification,
interpolation.yml, example "Dotted Names - Broken Chain Resolution".
(It is unfortunate that the current specification does not cover the
edge case of scalar sections; I proposed a PR to fix this in
mustache/spec#114 .)
Note: an equivalent template would already work
{{#var}} show the value {{.}} of the defined variable var {{/var}}
but it arguably results in less readable templates for non-experts.
gasche
added a commit
to gasche/ocaml-mustache
that referenced
this pull request
Dec 25, 2020
There was a bug in a corner case of the variable-lookup semantics,
which is important because it corresponds to a fairly natural "if
variable exists" pattern (in non-strict-mode). Consider the following
example when `var` is mapped to a scalar value in the context:
{{#var}} show the value {{var}} of the defined variable var {{/var}}
The current ocaml-mustache implementation will fail: when entiring the
`var` section, it would define the lookup context as the scalar value
itself, and any name lookup within the section would fail.
Instead the Mustache documentation and specification require that if
lookup in the current context fails (here the current context defines
no name), then "parent contexts" should be looked up; in particular,
the context before the section was entered, in which the lookup
succeeds.
> A {{name}} tag in a basic template will try to find the name key in
> the current context. If there is no name key, the parent contexts
> will be checked recursively. If the top context is reached and the
> name key is still not found, nothing will be rendered.
The present PR implements this "recursive lookup" semantics. Note that
falling back to parent contexts is *not* allowed in the middle of
resolving a dotted name (we find the right context for the first part
of the name, and then all subsequent lookups happen in
this context). This is explicitated in the Mustache specification,
interpolation.yml, example "Dotted Names - Broken Chain Resolution".
(It is unfortunate that the current specification does not cover the
edge case of scalar sections; I proposed a PR to fix this in
mustache/spec#114 .)
Note: an equivalent template would already work
{{#var}} show the value {{.}} of the defined variable var {{/var}}
but it arguably results in less readable templates for non-experts.
gasche
added a commit
to gasche/ocaml-mustache
that referenced
this pull request
Dec 25, 2020
There was a bug in a corner case of the variable-lookup semantics,
which is important because it corresponds to a fairly natural "if
variable exists" pattern (in non-strict-mode). Consider the following
example when `var` is mapped to a scalar value in the context:
{{#var}} show the value {{var}} of the defined variable var {{/var}}
The current ocaml-mustache implementation will fail: when entiring the
`var` section, it would define the lookup context as the scalar value
itself, and any name lookup within the section would fail.
Instead the Mustache documentation and specification require that if
lookup in the current context fails (here the current context defines
no name), then "parent contexts" should be looked up; in particular,
the context before the section was entered, in which the lookup
succeeds.
> A {{name}} tag in a basic template will try to find the name key in
> the current context. If there is no name key, the parent contexts
> will be checked recursively. If the top context is reached and the
> name key is still not found, nothing will be rendered.
The present PR implements this "recursive lookup" semantics. Note that
falling back to parent contexts is *not* allowed in the middle of
resolving a dotted name (we find the right context for the first part
of the name, and then all subsequent lookups happen in
this context). This is explicitated in the Mustache specification,
interpolation.yml, example "Dotted Names - Broken Chain Resolution".
(It is unfortunate that the current specification does not cover the
edge case of scalar sections; I proposed a PR to fix this in
mustache/spec#114 .)
Note: an equivalent template would already work
{{#var}} show the value {{.}} of the defined variable var {{/var}}
but it arguably results in less readable templates for non-experts.
gasche
added a commit
to gasche/ocaml-mustache
that referenced
this pull request
Dec 25, 2020
There was a bug in a corner case of the variable-lookup semantics,
which is important because it corresponds to a fairly natural "if
variable exists" pattern (in non-strict-mode). Consider the following
example when `var` is mapped to a scalar value in the context:
{{#var}} show the value {{var}} of the defined variable var {{/var}}
The current ocaml-mustache implementation will fail: when entiring the
`var` section, it would define the lookup context as the scalar value
itself, and any name lookup within the section would fail.
Instead the Mustache documentation and specification require that if
lookup in the current context fails (here the current context defines
no name), then "parent contexts" should be looked up; in particular,
the context before the section was entered, in which the lookup
succeeds.
> A {{name}} tag in a basic template will try to find the name key in
> the current context. If there is no name key, the parent contexts
> will be checked recursively. If the top context is reached and the
> name key is still not found, nothing will be rendered.
The present PR implements this "recursive lookup" semantics. Note that
falling back to parent contexts is *not* allowed in the middle of
resolving a dotted name (we find the right context for the first part
of the name, and then all subsequent lookups happen in
this context). This is explicitated in the Mustache specification,
interpolation.yml, example "Dotted Names - Broken Chain Resolution".
(It is unfortunate that the current specification does not cover the
edge case of scalar sections; I proposed a PR to fix this in
mustache/spec#114 .)
Note: an equivalent template would already work
{{#var}} show the value {{.}} of the defined variable var {{/var}}
but it arguably results in less readable templates for non-experts.
|
cc @spullara , @Danappelxx : I think this may be also an innocuous change worth merging. |
|
Thank you for the detailed commit message. Can you re-run |
This was referenced Mar 9, 2021
Closed
The behavior of "context stacks" is not explained very clearly in the documentation outside the spec, but the following text from mustache(5) (http://mustache.github.io/mustache.5.html) is pretty clear: > A {{name}} tag in a basic template will try to find the name key in > the current context. If there is no name key, the parent contexts > will be checked recursively. If the top context is reached and the > name key is still not found, nothing will be rendered. In particular, when the documentation of section mentions that > When the value is a non-empty list [..]. The context of the block > will be set to the current item for each iteration. or > When the value is non-false but not a list, it will be used as the > context for a single rendering of the block. it should be understood that the new context becomes the "current context", and that the previous context becomes a "parent context", and its binding are still available. The new tests check that these properties hold, in particular for sections on non-object non-false values, where this was not previously tested in the spec.
gasche
force-pushed
the
clarify-context-stacks
branch
from
61c0098
to
b04861a
Compare
|
Thanks! I rebased on top of master and ran |
softmoth
added a commit
to softmoth/raku-Template-Mustache
that referenced
this pull request
Mar 19, 2021
Thanks to an updated mustache-spec test for exposing this bug: mustache/spec#114
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some aspects of context stacks are not properly exercised by the current spec -- I realized this when hitting a bug in an implementation that was checked against the spec. When entering a new context, previous contexts should remain available for variables names that are not defined in the new contexts; this was only tested for object sections, not for list sections or non-false scalar sections.
In particular, inside a non-false scalar section
{{#name}}...{{/name}}(whose value is neither a list nor a string),{{.}}and{{name}}should always be equivalent.