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

API suggestion: hide string and i #6

Open
mratsim opened this issue Feb 9, 2021 · 3 comments
Open

API suggestion: hide string and i #6

mratsim opened this issue Feb 9, 2021 · 3 comments

Comments

@mratsim
Copy link

mratsim commented Feb 9, 2021

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.

@treeform
Copy link
Owner

treeform commented Feb 9, 2021

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

@PhilippMDoerner
Copy link
Contributor

PhilippMDoerner commented Jan 1, 2022

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).

@treeform
Copy link
Owner

treeform commented Jan 1, 2022

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.

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

3 participants