Better errors #51

gasche · 2020-12-26T14:19:10Z

This PR is on top of #50, it adds more informative arguments to render-time exceptions and transforms them into better error messages.

Before:

Fatal error: exception Mustache_types.Missing_variable("d")

After:

Template render error:
File "test.mustache", line 2, characters 7-12: the variable d is missing.

This is a breaking change as the exception types have changed, but the change only affects users who are not catching and inspecting Mustache exceptions.

However, good error locations are only available if people used the With_locations modules. If you want a 4.0 release for a breaking change, now may also be a good time to make With_locations the default.
(I have some other features planned for the next few days, so I am not suggesting to make a new release right now. But I'm interested in having an idea of the amount of breakage that @rgrinberg would consider acceptable for a future release.)

@rgrinberg I would have liked to add cram-style tests for this PR (I'm thinking of having bin/test where each error message is tested by cramming the binary), but this requires upgrading dune-lang to 2.7; I tried this and it complains that action was deleted some time ago and I should rewrite some other tests in a way that I don't really understand. When you have time, do you think that you could take care of upgrading dune-lang to 2.x for the project?

rgrinberg · 2020-12-26T18:21:01Z

However, good error locations are only available if people used the With_locations modules. If you want a 4.0 release for a breaking change, now may also be a good time to make With_locations the default.

Sounds good to me. Judging by opam, there aren't many users of this ilbrary, and the improvement in usability is significant.

When you have time, do you think that you could take care of upgrading dune-lang to 2.x for the project?

Sure.

gasche · 2020-12-26T21:17:02Z

bin/mustache_cli.ml

+    let lexbuf = Lexing.from_string template_data in
+    let () =
+      let open Lexing in
+      lexbuf.lex_curr_p <- { lexbuf.lex_curr_p with pos_fname = template_filename };


Note: this can now be done with just Lexing.set_filename lexbuf template_filename, but this function only appeared in 4.11 so I just went with the old way.

rgrinberg · 2020-12-26T22:07:50Z

bin/mustache_cli.ml

@@ -1,12 +1,27 @@
-let apply_mustache json_data template_data =
+module Mustache = struct


Why do we need this module?

This is a Mustache module where With_locations is the default. If I remove this declaration, Mustache.parse_lx and Mustache.render below will resolve to Mustache.Without_locations, so the errors will only receive dummy locations. Instead of defining this module, I could instead use Mustache.With_locations.parse_lx and Mustache.With_locations.render explicitly -- but I find the global declaration more convenient, as we never want to use the Without_locations definition in the binary.

(Which reminds me that I need to change the error-printing logic to avoid printing dummy locations. Will do right now.)

I thought our plan was to make good locations the default, in which case this would only be temporary. In any case, it's fine.

gasche · 2020-12-27T16:14:55Z

I rebased this PR on top of #55 (cram tests with examples of most error modes). I expected it to be useful for review, but it turns out to be very useful for myself as well, as I immediately spotted several easy improvements I wanted to make to the error messages I had chosen -- applied in the current PR.

The error message used the first example now says:

Template render error:
File "test.mustache", line 2, characters 7-12: the variable 'd' is missing.

(The change is to use single quotes around variable names; this is not so important with clearly-variable-names like d, but it makes a real difference when the missing variable looks more like a normal English word.)

- Invalid_template is now properly handled as a parsing error. - The various rendering exceptions are consolidated into a single error type, in preparation for better error payloads.

Before: Line 0, character -1: the variable title is missing. After: (At unknown location): the variable title is missing.

@gasche

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)

@gasche

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)

@gasche

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)

@gasche

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)

@gasche

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)

gasche force-pushed the better-errors branch from 5e97a76 to fc5f08f Compare December 26, 2020 15:24

gasche mentioned this pull request Dec 26, 2020

Track open sections #52

Closed

gasche mentioned this pull request Dec 26, 2020

Render with locations #50

Merged

gasche force-pushed the better-errors branch from b65782a to 7b99067 Compare December 26, 2020 21:15

gasche commented Dec 26, 2020

View reviewed changes

rgrinberg reviewed Dec 26, 2020

View reviewed changes

gasche force-pushed the better-errors branch from 7b99067 to 0bd9034 Compare December 27, 2020 05:12

This was referenced Dec 27, 2020

Cram tests for all error cases #55

Merged

Parsing errors on invalid templates are uncaught, unclear, and unhelpful #42

Closed

gasche force-pushed the better-errors branch 2 times, most recently from 20abeb6 to 1b08611 Compare December 27, 2020 16:05

rgrinberg approved these changes Dec 27, 2020

View reviewed changes

gasche added 4 commits December 27, 2020 20:42

[breaking] restructure exceptions

bf3ad00

- Invalid_template is now properly handled as a parsing error. - The various rendering exceptions are consolidated into a single error type, in preparation for better error payloads.

[refactor] harmonize the structure of parsing error payloads

26b6395

[refactor] extract a pp_loc function

282006e

richer payloads for render errors

d15eeb6

gasche force-pushed the better-errors branch 2 times, most recently from ae4a590 to 9151d9a Compare December 27, 2020 19:45

gasche added 5 commits December 27, 2020 20:47

add an error printer for render errors and register it

f06245a

use proper locations in mustache_cli and catch render errors

9df6265

mustache binary: include the input filename in locations

c7c090b

Specific location printing for dummy locations

38790f1

Before: Line 0, character -1: the variable title is missing. After: (At unknown location): the variable title is missing.

improve the error message on section mismatch

fb55977

gasche force-pushed the better-errors branch from 9151d9a to fb55977 Compare December 27, 2020 19:47

gasche merged commit 0600d37 into rgrinberg:master Dec 27, 2020

psafont mentioned this pull request Nov 24, 2023

[new release] mustache (3.2.0) ocaml/opam-repository#24839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better errors #51

Better errors #51

gasche commented Dec 26, 2020 •

edited

Loading

rgrinberg commented Dec 26, 2020

gasche Dec 26, 2020

rgrinberg Dec 26, 2020

gasche Dec 27, 2020 •

edited

Loading

rgrinberg Dec 27, 2020 •

edited

Loading

gasche commented Dec 27, 2020

		@@ -1,12 +1,27 @@
		let apply_mustache json_data template_data =
		module Mustache = struct

Better errors #51

Better errors #51

Conversation

gasche commented Dec 26, 2020 • edited Loading

rgrinberg commented Dec 26, 2020

gasche Dec 26, 2020

Choose a reason for hiding this comment

rgrinberg Dec 26, 2020

Choose a reason for hiding this comment

gasche Dec 27, 2020 • edited Loading

Choose a reason for hiding this comment

rgrinberg Dec 27, 2020 • edited Loading

Choose a reason for hiding this comment

gasche commented Dec 27, 2020

gasche commented Dec 26, 2020 •

edited

Loading

gasche Dec 27, 2020 •

edited

Loading

rgrinberg Dec 27, 2020 •

edited

Loading