-
Notifications
You must be signed in to change notification settings - Fork 34
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
fix #52: allow enum keys when parsing tables #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently jsony can only parse Table with string keys but I do not see any problem to allow a generic key.
I think there is one: a JSON key must be a string. That is, {1: "a"}
is invalid JSON.
See:
- The railroad diagrams on https://json.org
- https://www.ecma-international.org/wp-content/uploads/ECMA-404_2nd_edition_december_2017.pdf
To illustrate with other tools:
$ echo '{1: "a"}' | jq
parse error: Object keys must be strings at line 1, column 3
tests/test_tables.nim
Outdated
var s = """{1:"a"}""" | ||
var v = s.fromJson(Table[int, string]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s
is invalid JSON.
You are right, somehow I got convinced you could have any key (I am working on a yaml parser based on this, so things got mixed up ;)), thanks for the remark! I could fix to only allow string and enum as table keys. Suggestions welcome. |
fixed to restrict only to string and enum case. |
Not sure if the CI failure (failure when testing with js backend) is related to this fix, at the moment I do not have node locally to test it. |
I think your PR fails to due to issue: nim-lang/Nim#21317 |
fixed => nim-lang/Nim#21320 |
If CI is approved I would expect it to be green |
CI is green and ready to merge if approved |
sorry it took a while to address your comment, in the end the code was fine as it was but it took me a moment to realize that (see resolved comment above). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the final lines more clear? LGTM otherwise.
currently jsony can only parse Table with string keys but
I do not see any problem to allow a generic keyenum should be fine too. this fixes #52. added two test case for enum, the second of which replicates issue #52 (enum key). edited to remove possibility of generic keys.