-
Notifications
You must be signed in to change notification settings - Fork 10
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
refactor: use humantime and humansize for humanly units #25
Conversation
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.
Thanks a lot!
If it helps you, I am all for it!
Could you rewrite the commit to be refactor!: …
to indicate it's a breaking change?
Thanks for taking a look at my other comment as well.
display::Mode::with_percentage(), | ||
); | ||
assert_eq!( | ||
format!("{}", unit.display(100_002, Some(7_500_000), None)), | ||
"100.0k/7.5M objects [1%]" | ||
"100.0kB/7.5MB objects [1%]" |
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.
It looks like it now treats the amount of objects as amount of bytes. Can this be made so it's the same as before?
With none of these changes there should be a change in unit.
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.
My bad, didn't realize it's general units instead of specific sizes. I'll figure it out.
Oh I thought it's only an internal change so not breaking, but sure. |
You don't seem convinced, so I wasn't convinced anymore either as I just looked at the diff before. Now I checked more thoroughly and can confirm that |
It's not that I'm not convinced; my initial thoughts writing the change were "it's only replacing dependencies, no publicly visible changes", and my brain when replying was still booting up from sleep. You are right about |
This is the humantime part of PR Byron#25
With #26 merged, would this PR be superseded? To me it seems like it can be closed (or needs work). Thank you. |
Indeed. humansize is by name only for sizes, it would be inappropriate to jury rig it here anyway. Sorry for the delay - caught in work. |
Currently compound_duration and human_format are used. They don't see much use like the replacements, and the former has a 32-bit problem.
Another reason is that I, as a member of the Debian Rust team, hope to introduce less Rust crates into Debian, especially the less used ones. Packaging is more or less a manual process, unlike cargo publish which is automagic.