Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

json: add JsonKind, jsonKind (native json kinds) #353

Open
timotheecour opened this issue Mar 16, 2021 · 7 comments
Open

json: add JsonKind, jsonKind (native json kinds) #353

timotheecour opened this issue Mar 16, 2021 · 7 comments

Comments

@timotheecour
Copy link
Member

proposal

add these to std/json:

type JsonKind* {.pure.} = enum
  String
  Number
  Boolean
  Null
  Object
  Array

proc jsonKind*(self: JsonNode): JsonKind = ...

runnableExamples:
  import std/json
  block:
    let j = parseJson("184467440737095516159999999")
    assert j.kind == JString
    assert j.jsonKind == JsonKind.Number
  block:
    let j = parseJson("1.5")
    assert j.kind == JFloat
    assert j.jsonKind == JsonKind.Number

rationale

  • better than adding a dedicated API isNumber or exposing the private isUnquoted or adding isInteger (that would check kind and isUnquoted)

links

@arnetheduck
Copy link

When I want to get a Number out of the library, I would have to:

case node.jsonKind
of JsonKind.Number:
  case node.kind
  of JString: ...
  of JInt: ...

some convenience options should be added to work with jsonKind - in particular, getting the raw string out of there should always be smooth (because then I can pass it to a bigdecimal parser easily).

Also, a common issue in json libraries is that they only support integers up to 53 bits due to a float being used as the underlying representation, after which they start losing precision - as a solution to this, some libraries output strings instead of numbers for anything larger than the float-compatible cutoff - it would be nice to perhaps set the jsonkind as well to handle quirks like this.

@timotheecour
Copy link
Member Author

getting the raw string out of there should always be smooth (because then I can pass it to a bigdecimal parser easily).

the raw string is not available once you get a JsonNode and the original one is lost (eg we can't distinguish between different equivalent number representations). But you can already do $j:

case node.jsonKind
of JsonKind.Number:
  let str = $node

some convenience options should be added to work with jsonKind

what else?

some libraries output strings instead of numbers for anything larger than the float-compatible cutoff - it would be nice to perhaps set the jsonkind as well to handle quirks like this.

IMO the proper way to solve this and related problems is to add rendering options, either adding more options to json.pretty or adding a json.toString:

proc toString*(self: JsonNode, floatCutoff = false, indent = 2, multiline = true): string
let j = %(int64.high)
assert j.toString == "9223372036854775807"
assert j.toString(floatCutoff) == "\"9223372036854775807\"" # or maxBits = 53
assert j.toString(indent = 2) == ...

it would be nice to perhaps set the jsonkind as well to handle quirks like this.

I don't think we need to set the jsonkind, since a json string is used as the interchange format (not the JsonNode structure itself), we just need rendering options.

@metagn
Copy link
Contributor

metagn commented Mar 17, 2021

Is there a reason we can't add JBigNum that has a string field? Would it break too many things? Would it even fix problems?

@juancarlospaco
Copy link
Contributor

Is there a reason we can't add JBigNum

Someone needs to step up and make a BigInt for C target, theres already one for JS targets.

@timotheecour
Copy link
Member Author

timotheecour commented Mar 18, 2021

Is there a reason we can't add JBigNum that has a string field? Would it break too many things? Would it even fix problems?

unless I'm missing something, that's a separate problem; the point of this RFC is to introspect on the native json kind; (eg 1.2, 123, and 18446744073709551616 are both Number in json).

@arnetheduck
Copy link

BigInt

Of general interest here might be the json spec which doesn't really specify what a Number is, beyond offering a grammar for it - in particular though, it would have to be a BigDecimal in order not to lose precision - adding more libraries to the mix with their own quirks and constraints sounds like a difficult avenue that increases maintenance, opens up for platform-specific behavior and in the end might not actually implement json Numbers.

The key observation here is that parsing valid json according to the standard and mapping it to native language constructs are two separate things: a generic json library that focuses on allowing all json to be parsed probably shouldn't make too many sacrifices on the map-to-language front - that said, there are many json libraries out there that each have their own language-specific set of constraints and "improvements" like JInt etc, which is what makes json a bit hard to work with, when stepping outside the well-trodden float64 path with 53 bits of precision.

The other observation is that the specific desired mapping often depends on a more specific than json itself - the way for example uint64 is mapped to json in one application will differ from the way it should be mapped in another - this is somewhat solveable by not allowing smarter types than float near the json module, but with "generic" and open mappings that for example map all uint64 to a Number when doing import json make it hard to enforce at a compiler level and leaks happen which leads to bugs.

We're considering splitting off json into a separate library that can make a clean break - thoughts welcome - there are many benefits to this including independent development and release lifecycles for json vs the language.

@Araq
Copy link
Member

Araq commented Mar 23, 2021

Of general interest here might be the json spec which doesn't really specify what a Number is, beyond offering a grammar for it - in particular though, it would have to be a BigDecimal in order not to lose precision - adding more libraries to the mix with their own quirks and constraints sounds like a difficult avenue that increases maintenance, opens up for platform-specific behavior and in the end might not actually implement json Numbers.

I made very similar remarks when the problem was first reported but since then we focussed on a bugfix, I wasn't aware that it changed the behavior in a subtle way so the bugfix got backported. Mea culpa and the next time I'll ask Status for a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants