treeform/jsony

API suggestion: hide string and `i`

mratsim opened this issue · 3 comments

It's probably better to have the API hide s and i in

jsony/src/jsony.nim

Lines 13 to 22 in d45163b

proc parseHook*[T](s: string, i: var int, v: var seq[T])
proc parseHook*[T: enum](s: string, i: var int, v: var T)
proc parseHook*[T: object|ref object](s: string, i: var int, v: var T)
proc parseHook*[T](s: string, i: var int, v: var SomeTable[string, T])
proc parseHook*[T](s: string, i: var int, v: var SomeSet[T])
proc parseHook*[T: tuple](s: string, i: var int, v: var T)
proc parseHook*[T: array](s: string, i: var int, v: var T)
proc parseHook*[T: ref array](s: string, i: var int, v: var T)
proc parseHook*(s: string, i: var int, v: var JsonNode)
proc parseHook*(s: string, i: var int, v: var char)

This can be done with a lightweight parser object

type JsonParser = object
  view: openarray[char]
  pos: int

proc parseHook*[T](p: var JsonParser, v: var seq[T])
proc parseHook*[T: enum](p: var JsonParser, v: var T)
proc parseHook*[T: object|ref object](p: var JsonParser, v: var T)
proc parseHook*[T](p: var JsonParser, v: var SomeTable[string, T])
proc parseHook*[T](p: var JsonParser, v: var SomeSet[T])
proc parseHook*[T: tuple](p: var JsonParser, v: var T)
proc parseHook*[T: array](p: var JsonParser, v: var T)
proc parseHook*[T: ref array](p: var JsonParser, v: var T)
proc parseHook*(p: var JsonParser, v: var JsonNode)
proc parseHook*(p: var JsonParser, v: var char)

The API would be clearer instead of the user asking themself what that i parameter does and whether it was important or not.
It also gives you the ability to evolve the internals to add new functionality like a File field #5 or mmap support for large json files.

You are probably right about this. At first passing s and i was and still is conceptually simpler.

I stumbled over this just now as well and I think I understood it correctly that s is the JSON-string to parse and i is the index on that JSON string that you're currently at.
Is my understanding there correct?
I'd be very willing to throw in another PR for a minor README.md adjustment to document the way it is at the moment and also throw in a short list for which datatypes there's already parseHooks created (so you don't have to look it up in the source code).

I have a PR #37 that hides all of that behind json context. It still does not feel like the right approach. Passing a complex object around vs two simple things feels some how worse.