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

don't accept inexact numbers while parsing date values, closes #180 #181

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

OlegYch
Copy link
Contributor

@OlegYch OlegYch commented Sep 18, 2018

Pull Request Checklist

Fixes

Fixes #180

Purpose

Rejects inexact numbers as input for date parsers, they are limited to Long anyway

Background Context

BigDecimal.toLong takes inappropriate amount of time for something like 1000000000e1000000000

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you.

@OlegYch
Copy link
Contributor Author

OlegYch commented Sep 18, 2018

@dwijnand should i submit this against 2.6 branch too?

@dwijnand
Copy link
Member

I'm not sure, but I would think we probably want this fixed in 2.6 too. Whether or not that means a second PR.. I also don't know. 🙂

@marcospereira
Copy link
Member

Hi @OlegYch and @dwijnand,

Yes, we probably want to backport this to 2.6.x, but you won't need a new PR. We can cherry-pick on our side.

Copy link
Member

@marcospereira marcospereira left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you so much @OlegYch. I think we can have comprehensive tests for this fix. I'm also in doubt of a small change (see question).

Besides that, LGTM.

Best

Copy link
Member

@marcospereira marcospereira left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @OlegYch.

@marcospereira marcospereira merged commit 33558cb into playframework:master Sep 18, 2018
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.

3 participants