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

when lookup fail in the current context, go to parent contexts #49

Merged
merged 1 commit into from
Dec 25, 2020

Conversation

gasche
Copy link
Collaborator

@gasche gasche commented Dec 24, 2020

(This PR is on top of #48 to avoid conflicts in the rendering implementation.)

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.

@rgrinberg
Copy link
Owner

Looks good. A CHANGES entry would be helpful.

@gasche I added you as a collaborator. Feel free to merge approved PR's.

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

gasche commented Dec 25, 2020

Thanks! I rebased with a change entry, will merge if the CI passes.

@gasche gasche merged commit 2f1d46a into rgrinberg:master Dec 25, 2020
psafont added a commit to psafont/opam-repository that referenced this pull request Nov 24, 2023
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
psafont added a commit to psafont/opam-repository that referenced this pull request Nov 24, 2023
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
psafont added a commit to psafont/opam-repository that referenced this pull request Nov 27, 2023
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
psafont added a commit to psafont/opam-repository that referenced this pull request Nov 28, 2023
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
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.

2 participants