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

Fix regression in naturalsize for float #240

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Feb 17, 2025

Fixes #239.

@hugovk hugovk added the changelog: Fixed For any bug fixes label Feb 17, 2025
Copy link

codecov bot commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.49%. Comparing base (a0602c7) to head (11e62ee).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #240   +/-   ##
=======================================
  Coverage   99.49%   99.49%           
=======================================
  Files          11       11           
  Lines         796      796           
=======================================
  Hits          792      792           
  Misses          4        4           
Flag Coverage Δ
macos-latest 97.73% <100.00%> (ø)
ubuntu-latest 97.73% <100.00%> (ø)
windows-latest 95.97% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidboweninrupt
Copy link

davidboweninrupt commented Feb 17, 2025

Hi @hugovk

That's actually wonderful.

I can accept either behaviour, but I wanted it to be stable. Your additional tests really help with that. Ironically the change in behaviour popped up because my tests spotted it.

My local fix was also to convert the bytes to an integer.

I do have cases where I'm looking at the average-bytes-per-second so it might not be an integer. However, naturalsize does say it's trying to be like a file size so integers do make sense.

The proposed fix does an int rather than rounding it and as we know:

int(2.7)
2

which is tolerable, but not what I'd expect from humanize. That's mostly because I've been so impressed by how humanize has given me great outputs for so long. It looks like you are chatting about rounding down in naturaldelta too too.

So, in summary, I'd be fine with it going in this way.

I really appreciate you getting to this so quickly.

@hugovk
Copy link
Member Author

hugovk commented Feb 18, 2025

Yeah, and it matches the behaviour of the earlier percent formatting:

>>> "%d" % 2.7
'2'
>>> int(2.7)
2

I'm open to changing the rounding, but let's do it in a more intentional way than a bad formatting upgrade :)

@hugovk hugovk merged commit c11c08a into python-humanize:main Feb 18, 2025
35 checks passed
@hugovk hugovk deleted the fix-naturalsize branch February 18, 2025 16:48
@davidboweninrupt
Copy link

Nice, thanks @hugovk

@hugovk
Copy link
Member Author

hugovk commented Feb 19, 2025

You're welcome! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Fixed For any bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

naturalsize output changed between 4.11.0 and 4.12.0
2 participants