Skip to content

change clj long parsing logic#70

Merged
niwinz merged 1 commit intofuncool:masterfrom
danboykis:fix-long-parsing
Jan 9, 2019
Merged

change clj long parsing logic#70
niwinz merged 1 commit intofuncool:masterfrom
danboykis:fix-long-parsing

Conversation

@danboykis
Copy link
Contributor

do not coerce a long string into double which causes a loss of
precision

@danboykis
Copy link
Contributor Author

I got bit by this today:

For example:

(.longValue (Double/valueOf "31697972792222222"))
=> 31697972792222224
(Long/parseLong "31697972792222222")
=> 31697972792222222

@danboykis
Copy link
Contributor Author

danboykis commented Jan 7, 2019

This requires a bit more thought on my part since parse-long is pretty broken for java use cases.

@danboykis danboykis force-pushed the fix-long-parsing branch 2 times, most recently from 558c646 to 3ec3ae8 Compare January 8, 2019 04:15
@yogthos
Copy link

yogthos commented Jan 8, 2019

I would recommend a bug fix release for this as soon as possible as it's a major bug. The current implementation does not parse long values correctly, and the users will silently get wrong data when calling parse-int.

In the long term I think the name is misleading since the function doesn't just parse integers. It would be better to rename it to parse-number. I would also suggest throwing an error in case an unparsable string is pased in instead of returning a NaN. This is an error and it would be best to fail fast instead of quietly emitting a NaN value.

@niwinz
Copy link
Member

niwinz commented Jan 9, 2019

It is called parse-int with the clojure conventions. clojure treats longs and ints as the same type in most circumstances (see the integer? source as example).

You are right. I will issue a new bugfix release this afternoon.

@niwinz
Copy link
Member

niwinz commented Jan 9, 2019

In any case this PR, adds a duplicate parse-int function with wrong impl. I suggest submit an other PR (i'll fix it myself if this PR is not fixed for today)

In any case, thanks to all!.

@yogthos
Copy link

yogthos commented Jan 9, 2019

Thanks for the quick response. Regarding integer? it's a bit more nuanced because it's restricted to whole numbers even if the maximum size of a number is a long. A decimal number is not considered an integer, e.g: (integer? 1.3) => false. So, if the function is going to be called parse-int, it should be using #"-?\d+".

do not coerce a long string into double which causes a loss of
precision use BigDecimal instead
@danboykis
Copy link
Contributor Author

@niwinz I removed the duplicated parse-int function. I think the function is keeping with the spirit of what you tried to do in terms of using Double, but relying on BigDecimal instead.

@niwinz niwinz merged commit a4ed2a0 into funcool:master Jan 9, 2019
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