sile / jsone

Erlang JSON library
MIT License
291 stars 72 forks source link

Inline json array #16

Closed gootik closed 7 years ago

gootik commented 7 years ago

Fixes situations where an array of literal {json, _} tuples are passed for encoding.

sile commented 7 years ago

Unfortunately it conflicts with proplist based object representations (e.g., in master branch, <<"{\"json\":1}">> = jsone:encode([{json, 1}]) ).

I think your fix is reasonable, so would like to merge this PR if there are documents that explicitly describe that json_term() values are prefered than proplist based object representations.

Could you add above note to README.md ("Data Mapping" section?) and jsone.erl (e.g., doc comments for jsone_term/0 or json_object/0)?

pichi commented 7 years ago

Well, it is a very unintentional mistake I was made with json_term API. It is not a good solution. I think much better would be making jsonterm more special. What about make it `{{json, }}and{{jsonutf8, }}`? I know it is backward incompatible but avoids the problem with a proplist based object representations. I hope there are not many users of this feature yet so make this change quick. My fault.

gootik commented 7 years ago

Yeah I didn't even think abut propelist representations when making this change. @pichi are you suggesting changing it to

jsone:encode(#{ a => {{json, "[1,2,3]"}}}).

to get

{"a": [1,2,3]}
pichi commented 7 years ago

Yes, exactly. And

jsone:encode([{json, {{json, "[1,2,3]"}}}]).

should lead to

{"json": [1,2,3]}

When

jsone:encode([{json, <<"[1,2,3]">>}]).

should lead to

{"json":"[1,2,3]"}
gootik commented 7 years ago

@pichi @sile I've changed the code as per suggestions. I'm not sure if this is the best way to go, if people are using this already. You should probably add it to release note that there is a backwards incompatibility

pichi commented 7 years ago

@shezarkhani This feature is 13 days old and changes API without major version change. In normal SW cycle ti would be considered experimental feature anyway. I hope it has not gained a lot of users yet. I think proper fix is better than half fix. But it should be done fast.

sile commented 7 years ago

I merged https://github.com/sile/jsone/pull/17 and will close this PR.

Thanks for your contribution! > @shezarkhani , @pichi

gootik commented 7 years ago

:+1: thanks.