Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

Bug 754921: improve messages with invalid syntax #14

Merged
merged 1 commit into from
May 29, 2012
Merged

Bug 754921: improve messages with invalid syntax #14

merged 1 commit into from
May 29, 2012

Conversation

nickolay
Copy link
Contributor

  1. When JSON.parse() failed on invalid JSON (e.g. extraneous trailing
    comma) in ArgumentsJSON it raised a SyntaxError with no position
    information, which was displayed to the user as:

Syntax error at line , column : Unexpected token

  1. Also before this change all invalid parameter syntax (such as
    {{macro(1,)}} matched the ArgumentsJSON production, thus hitting the bug
    (1) described above.

  2. Also improved a helper in test-macros.js to help newbies (like
    myself) understand the structure of the testcase fixtures.

1) When JSON.parse() failed on invalid JSON (e.g. extraneous trailing
comma) in ArgumentsJSON it raised a SyntaxError with no position
information, which was displayed to the user as:

  Syntax error at line , column : Unexpected token

2) Also before this change all invalid parameter syntax (such as
{{macro(1,)}} matched the ArgumentsJSON production, thus hitting the bug
(1) described above.

3) Also improved a helper in test-macros.js to help newbies (like
myself) understand the structure of the testcase fixtures.
@nickolay
Copy link
Contributor Author

So with these changes you get message like these:

Syntax error at line 4, column 10: Expected "'", "\"", [ \t\n\r;] or [\-.0-9] but ")" found.
    2 | produces a sensible error message.
    3 | 
    4 | {{ add(1,) }}
-----------------^


Syntax error at line 7, column 3: Expected valid JSON object as the parameter of the preceding macro but "{\n    \"es\":\"es/JavaScript/Acerca_de_JavaScript\",\n    \"en\": \"en/JavaScript/About_JavaScript\",\n}" found.
    5 |     "es":"es/JavaScript/Acerca_de_JavaScript",
    6 |     "en": "en/JavaScript/About_JavaScript",
    7 | }) }}
----------^

You get even better error messages when defining the JSON grammar with PEG (by inlining https://github.com/dmajda/pegjs/blob/master/examples/json.pegjs ), but unfortunately

  • pegjs doesn't support including one grammar in another one (so we'd have to fork json.pegjs and modify it a little (|start|, |whitespace|))
  • json.pegjs has a FIXME about not supporting null values, which I'm not sure is acceptable for us.
  • the fix here is less regression-prone, since it (mostly) only affects failure cases.

@lmorchard
Copy link
Contributor

Tests pass, error messages improved in a manual test, hooray! Thanks for tracking that down!

lmorchard added a commit that referenced this pull request May 29, 2012
Bug 754921: improve messages with invalid syntax
@lmorchard lmorchard merged commit f573b3c into mdn:master May 29, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants