Conversation
Both functions converted the input with float() before trying int(),
only to test for NaN and infinity. For an integer beyond the float
range that conversion raises OverflowError, which is not in the
except clause, so it reached the caller instead of the documented
str(value) fallback -- despite the value being an ordinary int.
apnumber now tries int() first and falls back to float() only for
values that are not int-able, mirroring the fix applied to ordinal.
fractional still needs a float, so it keeps that conversion but now
treats an infinite result as genuine only when the value is not
itself an integer: float("1" + "0" * 400) returns inf rather than
raising, which previously reported a finite integer as "+Inf". An
integer that large has no fractional part to extract either way.
A literal "1e400" is still reported as +Inf, since it is not an
integer.
|
Automated AI review. Reproduced on humanize 4.16.0 and current main; the remaining PR behavior is inferred from the diff, not a run of this branch. One edge still appears uncovered: finite built-in integers above binary64’s exact-integer range but below float overflow. On current main, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #419
Changes proposed in this pull request:
apnumbernow triesint()beforefloat(), so an integer larger than the float range no longer raisesOverflowErrorfractionaldistinguishes a genuinely infinite value from an integer thatfloat()merely reports as infinite, so a finite integer is no longer rendered as+Infintand asstr, plus the"1e400"case that must still be+InfBoth functions converted the input with
float()purely to test for NaN and infinity, before doing theint()conversion they actually needed. For an integer beyond the float range that raisesOverflowError, which is not in theexcept (TypeError, ValueError)clause, so it reached the caller instead of the documented fallback:apnumberdocuments the intended behaviour explicitly — "always returns a string unless the value was notint-able, thenstr(value)is returned" — and10**400is int-able, so this is a contract violation rather than unsupported input.The string side had the mirror-image problem.
float("1" + "0"*400)returnsinfrather than raising, so a finite integer was reported as+Inf.fractionalstill needs a float, so it keeps that conversion but now only treats an infinite result as genuine when the value isn't itself an integer. An integer that large has no fractional part to extract either way.Behaviour after the change:
apnumberfractional10**400"1" + "0"*400-(10**400)"1e400"+Inf+Infinf/-inf/nan+Inf/-Inf/NaN0,5,1.0,"5.0"zero,five,one,5.00.5,1.5,"1/3"1/2,1 1/2,1/3"1e400"deliberately still reports+Inf: it isn't an integer, so it is genuinely infinite once parsed.Testing.
pytest tests/test_number.pygives 249 passed, and the full suite 737 passed / 112 skipped. I revertednumber.pywhile keeping the new tests and confirmed all six fail withOverflowError, so they are genuine regression coverage rather than decoration. (The 15 errors I see intests/test_benchmarks.pyarepytest-benchmarknot being installed locally, and occur onmaintoo.)Scope. #419 lists the same pattern in
intword,scientific,clampandmetric.ordinal,intcommaandintwordare already covered by #414, #404 and #415/#332, so this PR deliberately avoids them to prevent conflicts.scientificis left out on purpose — astr(value)fallback there would emit 401 digits from a function whose whole point is compact notation, so it likely wantsDecimal-based formatting (1.00 x 10⁴⁰⁰) instead. Happy to follow up with that separately if the approach sounds right.