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

Should allow primitive at root to comply RFC #102

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yasuoka
Copy link

@yasuoka yasuoka commented Mar 3, 2017

Current jsmn disallows only a primitive at root. But RFC7159 allows that like followings:

JSON-text = ws value ws
(snip)
value = false / null / true / object / array / number / string

So I think it should allow this even in JSMN_STRICT case.

@yasuoka yasuoka changed the title primitive at root Should allow primitive at root to comply RFC Sep 14, 2018
@pt300
Copy link
Collaborator

pt300 commented Apr 26, 2019

at some point it will be fixed

@yasuoka
Copy link
Author

yasuoka commented Apr 26, 2019

I rebased the test to https://github.com/zserge/jsmn , it resulted the following error
https://travis-ci.org/zserge/jsmn/builds/525143871
I noticed the latest doesn't allow primitives at root regardless JSMN_STRICT or not.

@pt300
Copy link
Collaborator

pt300 commented Apr 27, 2019

Don't worry, I'm slowly building a list of things that need to be added/changed/improved in the parses without breaking it. If all goes well and I have enough time I might push fix for that in at most a month or so.

@pt300
Copy link
Collaborator

pt300 commented Apr 27, 2019

although it should work without strict I believe... hmmm

test/tests.c Outdated
const char *js;
js = "\"hello\"";
check(parse(js, 1, 1,
JSMN_STRING, "hello"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For JSMN_STRING parse() expects 2 arguments, second being size, as in number of children. Just needs a 0 as argument after "hello".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! fixed it on the branch.

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

Successfully merging this pull request may close these issues.

None yet

2 participants