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

Correctly convert a Float to a JsNumber using BigDecimal#decimal #241

Merged

Conversation

lars-n
Copy link
Contributor

@lars-n lars-n commented Nov 14, 2017

When converting Floats to a JsNumber they will not be converted exact. The issue appears to be in JsNumber.apply (https://github.com/spray/spray-json/blob/master/src/main/scala/spray/json/JsValue.scala#L97) where the Float should be converted to a BigDecimal using BigDecimal.decimal rather than BigDecimal (see documentation there).

see #109

@ktoso
Copy link
Member

ktoso commented Nov 15, 2017

Verified that this indeed makes a difference, without it we'd get [error] '4.199999809265137' != '4.2' (BasicFormatsSpec.scala:46)

@ktoso
Copy link
Member

ktoso commented Nov 15, 2017

Thanks!

@ktoso
Copy link
Member

ktoso commented Nov 15, 2017

Failure seems to be MiMa, but not about any method that was touched in this PR... will fix separately

@ktoso
Copy link
Member

ktoso commented Nov 15, 2017

2.10 build failed.

This addition would mean that we're not compatible with Scala 2.10 anymore.
Which perhaps is fine, but we need to manage expectations properly about the next release then...

https://travis-ci.org/spray/spray-json/jobs/302117152

[error] /home/travis/build/spray/spray-json/src/main/scala/spray/json/JsValue.scala:97: value decimal is not a member of object scala.math.BigDecimal
[error]     case _                 => new JsNumber(BigDecimal.decimal(n))
[error]                                                       ^
[error] one error found

@lars-n
Copy link
Contributor Author

lars-n commented Nov 15, 2017

sry, forgot about 2.10. Changed the code to not use BigDecimal#decimal

@ktoso
Copy link
Member

ktoso commented Nov 15, 2017

Nice, AFAICS that's correct (and what BigDecimal.decimal does inside anyway), let's have the tests pass but looking good

@lars-n
Copy link
Contributor Author

lars-n commented Nov 15, 2017

tests seem to pass, but some other check failed

@lars-n
Copy link
Contributor Author

lars-n commented Nov 15, 2017

let me know if there is anything else that needs to be changed

@lars-n
Copy link
Contributor Author

lars-n commented Nov 27, 2017

@ktoso, any news on if/how/when this can be merged? It'd be useful for some cases here, not urgent though.

@jrudolph jrudolph changed the base branch from master to release/1.3.x November 10, 2020 10:03
@jrudolph jrudolph force-pushed the lars-n/convert-float-to-jsnumber branch from 543ca93 to c994751 Compare November 10, 2020 10:07
Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @lars-n even after all this time ;) (Rebased and removed whitespace changes)

@jrudolph jrudolph merged commit dc4ea94 into spray:release/1.3.x Nov 10, 2020
@lars-n
Copy link
Contributor Author

lars-n commented Nov 10, 2020

I already forgot about this one 😃 . Thanks @jrudolph

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