Added value unit/prefix parsing and conversion#1300
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1300 +/- ##
==========================================
- Coverage 55.70% 55.26% -0.44%
==========================================
Files 142 142
Lines 20558 20721 +163
Branches 5049 5060 +11
==========================================
+ Hits 11451 11452 +1
- Misses 5993 6155 +162
Partials 3114 3114
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
6adf4b5 to
d480215
Compare
456eb7f to
87877e0
Compare
src/main/core/support/units.rs
Outdated
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| match s { | ||
| "n" | "nano" => Ok(Self::Nano), | ||
| "u" | "micro" => Ok(Self::Micro), |
There was a problem hiding this comment.
It'd be kind of neat to also accept, and maybe display, μ. Maybe not worth bringing in unicode where we've previously only dealt with ascii though; up to you.
There was a problem hiding this comment.
Added (for both parsing and display).
| } | ||
|
|
||
| impl Prefixes for SiPrefixes { | ||
| fn relative_magnitude(&self) -> u128 { |
There was a problem hiding this comment.
Maybe a little more readable as:
let base = 1_000_000_000;
match self {
Self::Nano => base / 1_000_000_000,
Self::Micro => base / 1_000_000,
...
Self::Base => base,
...
Self::Tera => base * 1_000_000_000_000_000,
Self::Tibi => base * 1 << 40,
}
Same in the other tables below
There was a problem hiding this comment.
Added this base, but also am using pow() now to hopefully make it more readable.
src/main/core/support/units.rs
Outdated
|
|
||
| /// Parses a string as bytes-per-second. | ||
| #[no_mangle] | ||
| pub extern "C" fn parse_bandwidth(s: *const libc::c_char, out: *mut u64) -> bool { |
There was a problem hiding this comment.
Nit - maybe return an i64 and use -1 to signify errors? In particular this would help prevent a caller forgetting to check the RV going unnoticed (in the case that out already contained a plausible value).
src/main/core/support/units.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone, PartialEq)] | ||
| pub enum TimePrefixes { |
There was a problem hiding this comment.
This one is a little odd, since seconds, minutes, and hours aren't though of as prefixes. I was going to suggest maybe calling it TimeUnit, but maybe that'd be more confusing below. Maybe worth adding a comment about your reasoning either way
There was a problem hiding this comment.
Yeah, originally I wasn't going to include minutes and hours since they're different units, but then figured I'd add them even though they don't really fit, which meant adding the suffix to the prefixes. I have an implementation that splits it up into "prefixes", "units", and "values" (values have units, and units may or may not have prefixes), but I feel that it might be too confusing. For now I'll just add comments.
src/main/core/support/units.rs
Outdated
| use serde::de::{Deserialize, Deserializer, Visitor}; | ||
| use serde::ser::{Serialize, Serializer}; | ||
|
|
||
| pub trait Prefixes: Clone + Copy + Default + PartialEq + FromStr + Display + Debug { |
There was a problem hiding this comment.
Nit throughout - it's more conventional to name a trait, type, or enum after a member of that type. e.g. Prefix, SiPrefix
src/main/core/support/units.rs
Outdated
| use serde::de::{Deserialize, Deserializer, Visitor}; | ||
| use serde::ser::{Serialize, Serializer}; | ||
|
|
||
| pub trait Prefixes: Clone + Copy + Default + PartialEq + FromStr + Display + Debug { |
There was a problem hiding this comment.
Throughout - consider adding some doc comments to the pub elements.
There was a problem hiding this comment.
Added some more comments.
999e6ee to
956eaff
Compare
956eaff to
4930d53
Compare
This code is currently unused, but will be used in the future by the command line / configuration parsing. Part of #1161.