Add Filesize type
#14369
Add Filesize type
#14369
Conversation
IanManske
added
category:refactor
notes:fixes
| } else if *rhs == 0 { | ||
| if let Some(val) = checked_div_floor_i64(lhs.get(), rhs.get()) { | ||
| Ok(Value::int(val, span)) | ||
| } else if *rhs == Filesize::ZERO { |
There was a problem hiding this comment.
Since you do quite a bit of this, why not just add a .is_zero()? I think it's nicer to read
There was a problem hiding this comment.
Maybe, seems a little unnecessary though. Besides the filesize-specific methods, I only added methods also present on i64.
sholderbach
left a comment
sholderbach
left a comment
There was a problem hiding this comment.
Thanks for doing this tedious work!
(I was initially thinking for #14147 having that type just in FromValue would suffice but in Value its probably even better in the long term so folks don't reach for stupid mistakes)
| "B" => Self::B, | ||
| "kB" => Self::KB, | ||
| "MB" => Self::MB, | ||
| "GB" => Self::GB, | ||
| "TB" => Self::TB, | ||
| "PB" => Self::PB, | ||
| "EB" => Self::EB, | ||
| "KiB" => Self::KiB, | ||
| "MiB" => Self::MiB, | ||
| "GiB" => Self::GiB, | ||
| "TiB" => Self::TiB, | ||
| "PiB" => Self::PiB, | ||
| "EiB" => Self::EiB, |
There was a problem hiding this comment.
This is more strict than our existing case laissez-faire, where would this be applied?
There was a problem hiding this comment.
It's not used ATM. This is PR is split off of a larger refactor. I can move this to the follow up PR if that's more clear (forgot to remove it).
| /// ``` | ||
| pub const fn as_str(&self) -> &'static str { | ||
| match self { | ||
| Self::B => "B", |
There was a problem hiding this comment.
Not sure if this is also a concern for the NUON breaking change.
There was a problem hiding this comment.
It would be once it is used.
| @@ -1,13 +1,442 @@ | |||
| use crate::Config; | |||
| use crate::{Config, FromValue, IntoValue, ShellError, Span, Type, Value}; | |||
| use byte_unit::UnitType; | |||
There was a problem hiding this comment.
I am wondering if with all the code you have written, we can basically get rid of the byte_unit dependency? Last time I checked we only used a few parts with many of our own types or formatting on top.
There was a problem hiding this comment.
Yep, in my other branch, I was able to get rid of the dependency.
sholderbach
left a comment
sholderbach
left a comment
There was a problem hiding this comment.
To avoid bitrot I would vouch for landing this soon, I think the somewhat additional code (work you started for byte_unit replacement, right?) is fine here, if we can be clear about the impact on our case-insensitive parse and on the nuon hashing in nupm.
Fixes #16592 Maybe a more organized version of type Duration (like those of #14369) is better, but that's beyond this naive fix. ## Release notes summary - What our users need to know None ## Tasks after submitting None --------- Co-authored-by: Stefan Holderbach <[email protected]>
4 participants
Description
Adds a new
Filesizetype so thatFromValuecan be used to convert aValue::Filesizeto aFilesize. Currently, to extract a filesize from aValueusingFromValue, you have to extract ani64which coercesValue::Int,Value::Duration, andValue::Filesizeto ani64.Having a separate type also allows us to enforce checked math to catch overflows. Similarly, it allows us to specify other trait implementations like
Displayin a common place.User-Facing Changes
Multiplication with filesizes now error on overflow. Should not be a breaking change for plugins (i.e., serialization) since
Filesizeis marked withserde(transparent).Tests + Formatting
Updated some tests.