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

Improved assertion error messages on usage of JsonNode iterators on wrong kinds. #13389

Merged
merged 1 commit into from Feb 17, 2020

Conversation

@zevv
Copy link
Contributor

zevv commented Feb 11, 2020

The assertion alone is not helpful, the new message indicates what the user is doing wrong:

Error: unhandled exception: external/Nim/lib/pure/json.nim(755, 10) `node.kind == JObject` : 
pairs() can not iterate a JsonNode of kind JArray [AssertionError]
@alistairkeys

This comment has been minimized.

Copy link

alistairkeys commented Feb 11, 2020

There's a typo in the last assert message ("npairs()" instead of "mpairs()").

@zevv zevv force-pushed the zevv:zevv-json-iterator-errormessages branch from bc36b1e to 4793d14 Feb 11, 2020
@zevv

This comment has been minimized.

Copy link
Contributor Author

zevv commented Feb 11, 2020

There's a typo in the last assert message ("npairs()" instead of "mpairs()").

Thanks, fixed

@krux02

This comment has been minimized.

Copy link
Member

krux02 commented Feb 16, 2020

I think the assert should be removed entirely and iteration on nodes that are no arrays should be treated the same way as empty arrays. That is exactly how the node iterator in macros works, and it is very useful as one can skip the kind check when iterating children of a node. But that would be an RFC wouldn't it. Anyway, for the current assert, it is an improvement. Thank you. I think the test failure is unrelated. But I can't restart the job.

@Araq Araq merged commit 7355362 into nim-lang:devel Feb 17, 2020
7 of 8 checks passed
7 of 8 checks passed
builds.sr.ht: freebsd.yml builds.sr.ht job failed
Details
nim-lang.Nim Build #20200211.31 succeeded
Details
nim-lang.Nim (packages Linux_amd64) packages Linux_amd64 succeeded
Details
nim-lang.Nim (packages Linux_i386) packages Linux_i386 succeeded
Details
nim-lang.Nim (packages OSX_amd64) packages OSX_amd64 succeeded
Details
nim-lang.Nim (packages OSX_amd64_cpp) packages OSX_amd64_cpp succeeded
Details
nim-lang.Nim (packages Windows_amd64) packages Windows_amd64 succeeded
Details
nim-lang.Nim (packages Windows_amd64_pkg) packages Windows_amd64_pkg succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.