Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Proposal: support JSON+Comments #38

Closed
jamesarosen opened this Issue Mar 21, 2012 · 8 comments

Comments

Projects
None yet
4 participants
Contributor

jamesarosen commented Mar 21, 2012

JSON is a great format for information transport, but its lack of comments makes it bad for configuration files. The JSON+Comments spec aims to fix that. I would like to add an optional mixin (e.g. require "multi_json/minus_comments") that will strip out comments from input before passing it on to the underlying decoder.

Is anyone for or against? Any suggestions, opinions, or qualms?

Owner

mbleigh commented Mar 21, 2012

I think my preference would be to have it be an option passed into #decode (soon to be #load), e.g.

MultiJson.decode('{"some":"json"}', strip_comments: true)
Contributor

jamesarosen commented Mar 21, 2012

That looks grand :)

Member

sferik commented Mar 21, 2012

What is the existing behavior of the decoders? I would have assumed each one already strips out comments, but this must not be the case if you're proposing this change.

Overall, I'm in favor of this option. I just want to understand the existing behavior better. Could you start by writing some specs to verify what each engine currently does when parsing JSON+Comments?

Contributor

jamesarosen commented Mar 21, 2012

I added some specs in 8d233365e46. The oj and ok_json decoders both break:

Failures:

  1) MultiJson oj it should behave like an engine.decode strips //comments
     Failure/Error: MultiJson.decode(example).should == expected
     MultiJson::DecodeError:
       invalid format, expected , or } while in an object at line 2, column 15 [load.c:467]
     Shared Example Group: "an engine" called from ./spec/multi_json_spec.rb:69
     # ./lib/multi_json/engines/oj.rb:14:in `load'
     # ./lib/multi_json/engines/oj.rb:14:in `decode'
     # ./lib/multi_json.rb:81:in `decode'
     # ./spec/engine_shared_example.rb:130
     # ./spec/engine_shared_example.rb:120:in `each'
     # ./spec/engine_shared_example.rb:120

  2) MultiJson ok_json it should behave like an engine.decode strips //comments
     Failure/Error: MultiJson.decode(example).should == expected
     MultiJson::DecodeError:
       invalid character at "// the abc"
     Shared Example Group: "an engine" called from ./spec/multi_json_spec.rb:69
     # ./lib/multi_json/vendor/ok_json.rb:176:in `lex'
     # ./lib/multi_json/vendor/ok_json.rb:42:in `decode'
     # ./lib/multi_json/engines/ok_json.rb:10:in `decode'
     # ./lib/multi_json.rb:81:in `decode'
     # ./spec/engine_shared_example.rb:130
     # ./spec/engine_shared_example.rb:120:in `each'
     # ./spec/engine_shared_example.rb:120
Member

sferik commented Mar 21, 2012

IMHO, these are bugs in the individual engines.

Oj is a relatively new library and @ohler55 may be unaware of the bug. I'd recommend opening an issue at https://github.com/ohler55/oj/issues

Ditto for OkJson: https://github.com/kr/okjson/issues

Contributor

jamesarosen commented Mar 21, 2012

It seems most engines do strip comments, so I guess this can be taken up at the engine level.

Member

sferik commented Mar 21, 2012

Feel free to reopen this ticket if, for whatever reason, the engines are reluctant to implement this change.

Once these issues are resolved, let's remember to update Oj and OkJson and merge 8d23336. If you'd like to submit that pull request now, feel free to mark the spec as pending with links to the two open issues.

ohler55 commented Mar 21, 2012

I'll add an option to allow comments or just always ignore them in Oj. I'll be out this weekend but will try to get the feature in next week sometime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment