Conversation
- Conditions that check whether the SI or IEC units must be used were inverted, i.e., when `si_prefix == true` it would use `"iB"` instead of `"B"`. - KB & kiB were used instead of kB & KiB. - Switches (true/false) in tests are also fixed.
| #[derive(Debug, Clone, Default)] | ||
| pub enum Format { | ||
| #[default] | ||
| IEC, | ||
| SI, | ||
| } |
There was a problem hiding this comment.
this enum may or may not stay public (i have an idea to present in a follow up PR) but it really makes this PR easier to reason about
|
Thinking about these changes and #32, IMHO I would deprecate/remove |
src/serde.rs
Outdated
| fn test_serde_json() { | ||
| let json = serde_json::to_string(&ByteSize::mib(1)).unwrap(); | ||
| assert_eq!(json, "\"1.0 MiB\""); | ||
| assert_eq!(json, "\"1024.0 KiB\""); |
There was a problem hiding this comment.
Why is the output here "1024.0 KiB" instead of "1.0 MiB"?
There was a problem hiding this comment.
I'd put it down to a floating point error, but I agree that it's worth figuring out what changed. I notice now that the original PR didn't have to make this change 🤔
There was a problem hiding this comment.
The plot thickens...
assert_to_string("1.0 MiB", ByteSize::mib(1), false);
in the original PR fails the assertion.
There was a problem hiding this comment.
Solved! It was related to the floating point situation. I've increased the precision of the LN_ constants and now "1.0 MiB" is generated.
For sure. There's no way I'm letting this fn get in v2.0 😆 |
MrCroxx
left a comment
MrCroxx
left a comment
There was a problem hiding this comment.
LGTM. Thank you, Rob.
There is a tiny problem with the rust doc. Besides, we should setup a pipeline for doc test. Let me open an issue first.
| //! assert_eq!("482.4 GiB", ByteSize::gb(518).to_string_as(false)); | ||
| //! assert_eq!("518.0 GB", ByteSize::gb(518).to_string_as(true)); |
There was a problem hiding this comment.
The rust doc also needs to be updated.
There was a problem hiding this comment.
it will be, this fn still exists for now
4 participants
rebase and follow on from #38
Fixes #25
Fixes #26
Fixed #37