-
Notifications
You must be signed in to change notification settings - Fork 539
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
Permit Parsing Of Leading Zeros #1973
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1973 +/- ##
==========================================
+ Coverage 85.98% 86.08% +0.10%
==========================================
Files 74 74
Lines 2554 2544 -10
Branches 120 109 -11
==========================================
- Hits 2196 2190 -6
+ Misses 358 354 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@zarthross I can't seem to reproduce that failure locally, and I don't think it is related to this PR. |
bb0d3ee
to
01a5584
Compare
@zarthross errors gone on a re-run. The errors were relating to the Printer's sorting of object keys, but like I said, the didn't seem related to this PR. |
Perhaps it was #1911 you saw? |
This commit changes `BiggerDecimal.parseBiggerDecimalUnsafe` to permit parsing of leading '0' characters, and by extension allows all of our numeric codecs to tolerate decoding `JsonString` values which represent numbers with leading zeros. This is more in line with the `fromString` methods on the various numeric classes. Note, this change should be given some serious scrutiny. It appears that the original authors had at some time explicitly forbade this behavior, as noted by the existence of the `AFTER_ZERO` state as well as the explicit test case for invalid input including the `String` `"01"`. p I've searched around the issues and I couldn't find any reasoning for this decision, so I still think being lenient here is the right choice, but please double check me.
2cf9c26
to
a5ef603
Compare
@armanbilge I bet it was. |
@zarthross thoughts here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just 2 minor things I'd like fixed.
modules/numbers/shared/src/main/scala/io/circe/numbers/BiggerDecimal.scala
Outdated
Show resolved
Hide resolved
modules/numbers/shared/src/main/scala/io/circe/numbers/BiggerDecimal.scala
Outdated
Show resolved
Hide resolved
modules/numbers/shared/src/main/scala/io/circe/numbers/BiggerDecimal.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the minor naming nitpick I think this looks good.
For the record I think accepting non-standard JSON is a mistake (except maybe in some rare cases via a custom parser, but definitely not the default parser). |
Hey Travis, while I would generally agree with you, this is only impacting the string based json numbers decoding (which I don’t think is in the spec), and not json parsing. I think this change is fine, especially given the stdlib parse methods like |
This commit changes
BiggerDecimal.parseBiggerDecimalUnsafe
to permit parsing of leading '0' characters, and by extension allows all of our numeric codecs to tolerate decodingJsonString
values which represent numbers with leading zeros. This is more in line with thefromString
methods on the various numeric classes.Note, this change should be given some serious scrutiny. It appears that the original authors had at some time explicitly forbade this behavior, as noted by the existence of the
AFTER_ZERO
state as well as the explicit test case for invalid input including theString
"01"
.I've searched around the issues and I couldn't find any reasoning for this decision, so I still think being lenient here is the right choice, but please double check me.