Skip to content

fix: carry filesize suffix when mantissa rounds up to base#600

Open
patchwright wants to merge 2 commits into
PyFilesystem:masterfrom
patchwright:fix/filesize-rollover
Open

fix: carry filesize suffix when mantissa rounds up to base#600
patchwright wants to merge 2 commits into
PyFilesystem:masterfrom
patchwright:fix/filesize-rollover

Conversation

@patchwright

Copy link
Copy Markdown

Problem

All three public functions (traditional, binary, decimal) can produce impossible output like "1,024.0 KB" instead of "1.0 MB" for values just below a unit boundary.

Root cause: _to_str picks the suffix from the unrounded value, then formats the mantissa — which can round up to base, producing an impossible result.

filesize.decimal(999_999)        # '1,000.0 kB'  ← should be '1.0 MB'
filesize.traditional(1024**2-1)  # '1,024.0 KB'  ← should be '1.0 MB'
filesize.binary(1024**2-1)       # '1,024.0 KiB' ← should be '1.0 MiB'

Fix

Convert suffixes to a list before the loop. After computing the mantissa, round and compare against base. If the rounded value hits base, step up one suffix and recompute:

mantissa = base * size / unit
if round(mantissa, 1) >= base and i - 1 < len(suffixes):
    suffix = suffixes[i - 1]
    mantissa = size / unit

Math: when the current unit is base**i, dividing size by that same unit gives the value in the next suffix (whose denominator is base**(i+1), which divides into base * size / base**(i+1) = size / base**i = size / unit).

Also removes the existing TODO comment that flagged this exact issue.

Tests

Added test_rollover_decimal, test_rollover_traditional, and test_rollover_binary covering the unit boundaries for all three functions.

@lurch

lurch commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Ahhh, the fun of floating-point rounding 😆

I wonder if it might also be worth adding some unit-tests for the boundary-values? E.g.

>>> traditional(1996)
'1.9 KB'
>>> traditional(1997)
'2.0 KB'
>>> decimal(1950)
'1.9 kB'
>>> decimal(1951)
'2.0 kB'
>>> binary(1996)
'1.9 KiB'
>>> binary(1997)
'2.0 KiB'

@patchwright

Copy link
Copy Markdown
Author

Good call, those within-unit boundary cases are worth pinning down separately from the rollover ones. Added a test_rounding_boundaries covering your exact examples for all three functions:

traditional(1996) == "1.9 KB"
traditional(1997) == "2.0 KB"
decimal(1950)     == "1.9 kB"
decimal(1951)     == "2.0 kB"
binary(1996)      == "1.9 KiB"
binary(1997)      == "2.0 KiB"

All pass on the branch. Thanks for the review.

@lurch

lurch commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Is it also worth checking the boundaries at e.g.

>>> decimal(999_949)
'999.9 kB'
>>> decimal(999_950)
'1.0 MB'

or do you think those are sufficiently covered by the existing tests?

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.

2 participants