search

michalmuskala / jason

A blazing fast JSON parser and generator in pure Elixir.
Other
1.6k stars 170 forks source link

Add value and decoded types and typedocs. #129

Open azizk opened 3 years ago

azizk commented 3 years ago

Hi!

I'd like to propose adding two types. Namely value and decoded. I sometimes noticed that it would be great to have a JSON value type handy, when I was dealing with JSON data. I'm sure others would benefit as well when they can refer to a canonical type from the most popular JSON Elixir library itself (e.g. @spec f(Jason.value()) :: Jason.value()).

I gave the input parameters the decoded type so that dialyzer may catch instances where developers unknowingly pass a tuple to the encode functions. But I'm not sure if that really helps or if it will be more of a hindrance.

I distinguish the value type from the decoded type, which can have any kind of map keys, so that we can have a stricter and more basic type when we're dealing with JSON data. :+1:

PS: You'll notice I changed String.t() to binary. It's a matter of preference to me and we can change it back to String.t() of course.

michalmuskala commented 3 years ago

Unfortunately using the decoded type as input to the encode functions is incorrect since any value implementing the Jason.Encoder protocol can be passed - in fact you could implement it for tuples as well, it's just that it's not implemented by default (since there's no good default representation for tuples in JSON). You can't really do better than term as input there. Additionally, Jason operates only over valid utf-8, which is signalled by using the String.t type, rather than arbitrary binary.

azizk commented 3 years ago

Oh I see. But do you think it would be a reasonable assumption to make that normally hardly anyone will implement the encoder protocol for tuples? I don't know, maybe in 1% of all cases someone might want to, but in those cases they probably need to bring the data into an encodable shape anyway before they pass it to Jason.encode(). So perhaps it would be better to be on the safer side for the majority of usage cases. So maybe we should allow everything in the encode spec except for tuples?

In any case, what do you think about restricting the output type of the decode function? Looks good to me as far as I can see.

PS: I made some additional commits to improve things.

azizk commented 3 years ago

By the way, the term type includes pid, port, reference, function, BitString etc. Maybe it's worth it to restrict the encodable type somewhat?

azizk commented 3 years ago

Hey @michalmuskala, I changed the code so that encodable is basically a term again. I made some other changes too. I hope it's possible for you to review and sign it off this time? Appreciate your time.