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

Use upper-bound correctly in parse-number fn #125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dvdreddy
Copy link

@dvdreddy dvdreddy commented Feb 9, 2018

  • Lower / Upper bound was not being correctly enforced when trying to read a number
  • Also force the default radix to be 10 to avoid irregular behavior of the parse fn

Fixes #124

* Lower / Upper bound was not being correctly enforced when trying to read a number
* Also force the default radix to be 10 to avoid irregular behavior of the parse fn
@danielcompton
Copy link
Collaborator

Would it be good to add some tests exercising this change?

@dvdreddy
Copy link
Author

dvdreddy commented Feb 9, 2018

Just for the parse-number I can surely add,

But for overall parsing I tried to come up with a case which fails the formatting for date-time, I came up short mainly because the current code works fine due to the separators.

@danielcompton
Copy link
Collaborator

If you can write some tests for parse-number that fail under the old implementation, that would be great.

But for overall parsing I tried to come up with a case which fails the formatting for date-time, I came up short mainly because the current code works fine due to the separators.

Can you explain this a little more? I think I might know what you mean, but it'd be good to have an example to make sure we're on the same page.

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.

2 participants