Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Allow lift-json to parse primitives at the top level #1363

Merged
merged 3 commits into from Nov 20, 2012

Conversation

Projects
None yet
2 participants
Contributor

frj commented Nov 19, 2012

Hi,

Ive always found it annoying that you can parse strings, ints, bools etc as standalone entities.

Eg:

parse("12345")

Doesnt work.

I realise that this request may not be 100% to spec, but I noticed lift json will 'write' a JString on its own and was hoping you might apply this change.

Thanks,

Franis

I'm fine with the feature change (although JSON spec does not allow primitives at top level). Two issues with this patch though:

  • Boxing to Option at parsing level must have a performance impact. That should be measured or boxing should be removed.
  • Why 'forAll(genJValue)(parsing)'

Francis Rhys-Jones added some commits Nov 20, 2012

Contributor

frj commented Nov 20, 2012

Hi there,

Thats great thanks!

Your right there probably would be some performance Impact, the additional memory allocation would have gc implications, its a little difficult to assess impact in real world situations.

Ive removed the boxing, I do a empty check, which looking at the implementation in linked list looks fairly cheap.

I could have done a match on null, however this might be considered heresy by some, but would avoid the additional size check. I can do that if you think it would be a better trade off.

As for the forAll(genJValue)(parsing), In my limited understanding of the code you are using to test the parser, this would generate a set of documents including 'simple' ones ie containing just a single primitive.

This did indeed generate the failure case I was looking for, but please do give me a steer on the best way to test this change.

Thanks again,

Francis

Member

jonifreeman commented Nov 20, 2012

Looks good to me. I see why you added genJValue there now, makes sense. Thanks for the patch!

@jonifreeman jonifreeman added a commit that referenced this pull request Nov 20, 2012

@jonifreeman jonifreeman Merge pull request #1363 from frj/master
Allow lift-json to parse primitives at the top level
f4307f6

@jonifreeman jonifreeman merged commit f4307f6 into lift:master Nov 20, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment