Fixed support for keys that look like numbers longer than an int. #568
Conversation
|
It was only after I made the pull request that I realized there is already an issue and a pull request open with regard to the issue I tried to solve here ( #557 ). While I greatly appreciate the effort put in the other pull request (as it definitely develops the library's extensibility in the right way), I wonder if my fix couldn't be used for a bugfix release? As I believe the Therefore, I believe my fix is rather straightforward and could be merged for a bugfix release much sooner, with little need for further discussion. The final solution, as developed in the other pull request/issue, would in time replace my fix, while not leaving a glaring bug in the library in the meantime. What do you think? |
|
Sorry for the slow reply; I agree with separating out just the bugfix, but maybe using BigInteger is a nicer solution than checking the number of digits (so that we still sort things as numbers even when large)? |
|
I am not sure why did the Travis CI build fail, the only error message I see is "We couldn't find the repository lightbend/config" but I don't know if it's me not having some permissions to see the build or is it an actual configuration problem? |
|
Maybe a transient travis bug? Anyway the error is info] Set current project to config-root (in build file:/home/travis/build/lightbend/config/) |
|
OK, now I see the Travis logs as well! But it's the same error as in #602 – an unrelated issue with the Scala version used by Travis CI (2.12.8, as seen in the error message) vs the one defined in SBT for the project and (I guess) the recent release of Scala 2.12.8. I guess it should be an easy fix by either specifying the Scala version to use in the |
|
I'm not sure of the right fix but agree it should be in its own PR. Bumping the project version of scala seems like it would keep the build faster (not need to download another one), but it could have some unforeseen complexity to it, I don't know. |
| if (Character.isDigit(c)) | ||
| continue; | ||
| else | ||
| if (!Character.isDigit(c)) |
…ngs fixed As discussed in lightbend#568 . `lift-json` was also bumped as there was no longer any suitable version for Scala 2.12.x. Also fixed compiler warnings (e.g. _eta expansion of parameterless methods is deprecated_) as well as some rarther non-controversial IDE warnings (e.g. redundant semicolons and such).
…ngs fixed As discussed in lightbend#568 . `lift-json` was also bumped as there was no longer any suitable version for Scala 2.12.x. Also fixed compiler warnings (e.g. _eta expansion of parameterless methods is deprecated_) as well as some rarther non-controversial IDE warnings (e.g. redundant semicolons and such).
Previously such config keys would still be parsed with `Integer#parseInt` for the sake of sorting, resulting in a `NumberFormatException`. Now the keys consisting of digits only are parsed to a `BigInteger` and compared as such. This addresses the following issues: lightbend#604 lightbend#541
|
Travis is finally at peace :) |
|
🎉 thanks! |
Previously such config keys would still be parsed with
Integer#parseInt, resulting in aNumberFormatException.Now the number-like keys are parsed to a Long (no measures made but
performance expected hit expected to be negligible) if their numbers of
digits is shorter than the number of digits in
Long.MAX_VALUE.One could have also tried to parse numbers exactly up to
Long.MAX_VALUE,but added usability doesn't seem to match the required complexity - in
fact, one could also argue that the current loop-based checking if a key
looks like a number is a bit of an overkill, but I didn't want to dig into
the original reason for why a regexp was not used, instead focusing on
fixing an apparent bug in the most straightforward way.