Remove the ast without locations #65
Merged
Conversation
This file contains hidden or 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
rgrinberg
approved these changes
Jan 5, 2021
gasche
force-pushed
the
remove-the-ast-without-locations
branch
from
5ece0c8 to
791ee57
Compare
This change substantially simplifies the codebase by getting rid of the duplication of AST definitions. Now there is only one AST, the AST with locations. For compatibility reasons we still have With_locations and Without_locations module, and Without_locations is still the one exported by default. This means that users that programmatically build templates through the smart constructors see no change -- they are just passing dummy-locations everywhere. (If they were building AST fragments using the variant constructors directly, they can switch to the smart constructors and have code that is compatible with both the old and new version of Mustache. If they were pattern-matching on AST fragments, I would suggest updating the codebase instead of trying to use 'fold'.) The new 'compatibility test' still passes, suggesting that this indeed preserves reasonable (minimal) compatibility. In the longer term, we could decide to get rid of Without_locations entirely and expose the With_locations interface by default. However, it is not so clear that this is really important (if I build an AST fragment programmatically, I probably don't care about the locations), and it may require some slight API changes (making the location argument optional?) The present change gets most of the practical benefits of switching to the With_locations world: all users now get proper locations in their error messages (not just With_locations users). (Notice the related simplification in mustache_cli.ml.)
gasche
force-pushed
the
remove-the-ast-without-locations
branch
from
791ee57 to
25acacc
Compare
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
2 participants
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.
This change (on top of #64) substantially simplifies the codebase by getting rid of
the duplication of AST definitions. Now there is only one AST, the AST
with locations.
For compatibility reasons we still have With_locations and
Without_locations module, and Without_locations is still the one
exported by default. This means that users that programmatically build
templates through the smart constructors see no change -- they are
just passing dummy-locations everywhere.
(If they were building AST fragments using the variant constructors
directly, they can switch to the smart constructors and have code that
is compatible with both the old and new version of Mustache. If they
were pattern-matching on AST fragments, I would suggest updating the
codebase instead of trying to use 'fold'.)
The new 'compatibility test' still passes, suggesting that this indeed
preserves reasonable (minimal) compatibility.
In the longer term, we could decide to get rid of Without_locations
entirely and expose the With_locations interface by default. However,
it is not so clear that this is really important (if I build an AST
fragment programmatically, I probably don't care about the locations),
and it may require some slight API changes (making the location
argument optional?)
The present change gets most of the practical benefits of switching to
the With_locations world: all users now get proper locations in their
error messages (not just With_locations users). (Notice the related
simplification in mustache_cli.ml.)
(cc @Armael, the initial architect of the With_locations transition)