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

(2.6 and 3.0) fix json parser e+ bug #1492

Closed
wants to merge 1 commit into from

Conversation

kluyg
Copy link

@kluyg kluyg commented Sep 10, 2013

According to http://www.json.org/ the double in JSON can be represented in scientific notation, and e symbol can be one of the following:
e
e+
e-
E
E+
E-
net.liftweb.json.parse was working ok with e, e-, E, E- but was throwing exception "can't parse" for e+ and E+.
Added unit test (was failing). Added c == '+' analogous to c == '-' to JsonParser.scala. This fixed unit test.
P.S. The same issue exists in master, but we use 2.4 so I fixed it in 2.4 branch. The change is atomic and can be merged to any lift version, including master.

@Shadowfiend
Copy link
Member

Given that this isn't a mission-critical bug, I don't know that it merits a special release of 2.4.

👍 for the fix to land in 2.6 and 3.0, however.

@Shadowfiend
Copy link
Member

Er… Actually… Before I forget… Did you discuss this on the mailing list before issuing the PR? It doesn't look like it. Please read the contribution guidelines, which are listed whenever you open an issue or pull request.

@kluyg
Copy link
Author

kluyg commented Sep 10, 2013

@Shadowfiend Ups, sorry about that. It's my very first pull-request. Just wrote to the mailing list: https://groups.google.com/forum/#!topic/liftweb/dvlgelrPfFg
contributors.md doesn't exist in 2.4 branch.
Should I remove this pull-request and prepare another one for "master" branch after discussion in mailing list and with contributors.md changes?

The change is pretty small, but the issue is critical to me - I use lift-json to parse json from third-party services and one of them returns numbers in this format, which is valid json, but can't be parsed by lift-json.

By submitting this pull request which includes my name and email address
(the email address may be in a non-robot readable format), I agree that the
entirety of the contribution is my own original work, that there are no prior
claims on this work including, but not limited to, any agreements I may have with
my employer or other contracts, and that I license this work under
an Apache 2.0 license.

Name:

Mikhail Strebkov

Email:

strebkov @@ gmail .. com

@dpp
Copy link
Member

dpp commented Sep 10, 2013

On Tue, Sep 10, 2013 at 8:04 AM, kluyg notifications@github.com wrote:

@Shadowfiend https://github.com/Shadowfiend Ups, sorry about that. It's
my very first pull-request. Just wrote to the mailing list:
https://groups.google.com/forum/#!topic/liftweb/dvlgelrPfFg
contributors.md doesn't exist in 2.4 branch.

Prior to the 2.5 release, we only accepted contributions from committers.
We relaxed that with the 2.5 release.

Should I remove this pull-request and prepare another one for "master"
branch after discussion in mailing list and with contributors.md changes?

We will only roll this into Master and lift_30. We will not backport it
(unless you are willing to pay for the time it takes to roll new versions
of prior releases of Lift.) The only thing we backport is critical security
fixes for which there are no reasonable work-arounds.

Also, we'll have to wait for Joni Freeman (who owns the lift-json module)
to weigh in on the issue.

The change is pretty small, but the issue is critical to me - I use
lift-json to parse json from third-party services and one of the them
returns numbers in this format, which is valid json, but can't be parsed by
lift-json.

By submitting this pull request which includes my name and email address
(the email address may be in a non-robot readable format), I agree that the
entirety of the contribution is my own original work, that there are no
prior
claims on this work including, but not limited to, any agreements I may
have with
my employer or other contracts, and that I license this work under
an Apache 2.0 http://www.apache.org/licenses/LICENSE-2.0.html license.
Name:

Mikhail Strebkov
Email:

strebkov @@ gmail .. com


Reply to this email directly or view it on GitHubhttps://github.com//pull/1492#issuecomment-24167226
.

Telegram, Simply Beautiful CMS https://telegr.am
Lift, the simply functional web framework http://liftweb.net
Follow me: http://twitter.com/dpp
Blog: http://goodstuff.im

@kluyg
Copy link
Author

kluyg commented Sep 10, 2013

Prior to the 2.5 release, we only accepted contributions from committers.
We relaxed that with the 2.5 release.

I see. Great that this was relaxed.

We will only roll this into Master and lift_30. We will not backport it
(unless you are willing to pay for the time it takes to roll new versions
of prior releases of Lift.) The only thing we backport is critical security
fixes for which there are no reasonable work-arounds.

I see. I can live with my own jar's until lift_30.

So should I remove pull-request and prepare another one for master? Or remove and wait for Joni Freeman?

@jonifreeman
Copy link
Member

Looks good to me. Double checked the spec too and [num] e+ [exp] is a valid representation for a decimal number.

@dpp
Copy link
Member

dpp commented Sep 10, 2013

Joni -- Thanks for your thumbs up.

Let's get this into master and I'm pretty sure one of the code gnomes will
move it to lift_30 as well.

On Tue, Sep 10, 2013 at 9:16 AM, Joni Freeman notifications@github.comwrote:

Looks good to me. Double checked the spec too and [num] e+ [exp] is a
valid representation for a decimal number.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1492#issuecomment-24173870
.

Telegram, Simply Beautiful CMS https://telegr.am
Lift, the simply functional web framework http://liftweb.net
Follow me: http://twitter.com/dpp
Blog: http://goodstuff.im

@fmpwizard
Copy link
Member

rebased to master, thanks!

@fmpwizard fmpwizard closed this Sep 15, 2013
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

5 participants