groupon / cson-parser

Simple & safe CSON parser
BSD 3-Clause "New" or "Revised" License
133 stars 27 forks source link

Add parse-json dependency #58

Closed Kikobeats closed 8 years ago

Kikobeats commented 8 years ago

The parse-json dependency reports more helpful errors.

jkrems commented 8 years ago

Can you give an example of the change in error message? At that point I would be surprised if there are any parse errors, given that it already passed coffee-script's own parser.

Kikobeats commented 8 years ago

@jkrems of course. You can see it on action in the parse-json documentation

before:

JSON.parse(json);
/*
undefined:3
}
^
SyntaxError: Unexpected token }
*/

after:

parseJson(json);
/*
JSONError: Trailing comma in object at 3:1
}
^
*/

Basically JSON.parser errors are too generic and not precise and that's a problem when you have a transpiler layer over your code

jkrems commented 8 years ago

Sorry, should have been more specific: I did read the README of parse-json. The question was more about how it would affect the line you changed. A Literal should only contain perfectly valid simple (string/number/etc.) literals, the coffee-script parser should ensure that.

So what concerns me is that there can be a SyntaxError in that line in the first place. It's supposed to only deal with simple, valid literals. It shouldn't ever even try to parse undefined or {"any":"object literal"}.

That's why I asked for an example where this change makes a difference to the behavior of CSON.parse.

Kikobeats commented 8 years ago

then I didn't understand the code. The thing that I was trying to improve is when you provide as input a JSON to CSON.parse.

jkrems commented 8 years ago

Okay, so the current behavior is:

> node -p "require('.').parse('undefined')"
/Users/jkrems/Projects/itier/libs/cson-parser/lib/parse.js:220
      throw new SyntaxError(syntaxErrorMessage(csNode, "Unexpected " + type));
      ^

SyntaxError: Syntax error on line 1, column 1: Unexpected UndefinedLiteral

And for trailing comma (which is legal in CSON):

> node -p "require('.').parse('{\"x\":1,\"y\":2,}')"
{ x: 1, y: 2 }

So I assume this PR can be closed because the current behavior already matches what you'd want..?

Kikobeats commented 8 years ago

Absolutely