Implement TryFrom<Time> for Duration #150
Conversation
|
I started reviewing (and making changes) yesterday and ran out of time. I pushed the changes to https://github.com/iliekturtles/uom/tree/Aehmlo-duration. Bunch of formatting changes, Right now I'm in agreement with you that adding a build.rs for rustc version checking goes a bit overboard and I'm leaning towards a feature. I haven't fully thought this through so I'm very open to arguments either way. |
|
After a brief reading of the code, I think the changes all make sense. I expect I'll have some time this week to look over it more fully and finish the thing up. I'll use a feature for now and we can revisit it later if need be. |
|
Something I just thought of: like temperature and temperature difference, there's a distinction between time and time interval (see |
|
Aren't instant/duration higher level concepts? Similar to a point and a length vector in one dimensional space. |
|
I suppose that an instant is meaningless without some reference, so yeah, punting and letting consumers deal with this is probably best. It feels a little impure semantically, but since the omission of a |
Aehmlo
force-pushed
the
duration
branch
2 times, most recently
from
0f5fa91 to
1242bbd
Compare
|
Sorry, fighting with Working Copy. The real thing should be ready soon. |
|
Okay, it looks like I'm going to need to fix the commit history when I get back to my Mac, but the changes at Aehmlo/uom:duration are ready for review. |
|
Reviewed! Diff below of (most? all?) of the requested changed. EDIT: Run
diff --git a/.travis.yml b/.travis.yml
index e6f1c72..9858d0d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -22,11 +22,11 @@ matrix:
rust: stable
script: |
set -e
- cargo test --all --verbose --features "use_serde"
- cargo test --manifest-path tests/edition_check/Cargo.toml --verbose --features "use_serde"
+ cargo test --all --verbose --features "use_serde try-from"
+ cargo test --manifest-path tests/edition_check/Cargo.toml --verbose --features "use_serde try-from"
cargo test --verbose --no-default-features --features "f32 si"
- cargo test --verbose --no-default-features --features "autoconvert f32 si use_serde"
- cargo test --verbose --no-run --no-default-features --features "autoconvert usize isize bigint bigrational si std use_serde"
+ cargo test --verbose --no-default-features --features "autoconvert f32 si use_serde try-from"
+ cargo test --verbose --no-run --no-default-features --features "autoconvert usize isize bigint bigrational si std use_serde try-from"
cargo test --verbose --no-run --no-default-features --features "autoconvert usize u8 u16 u32 u64 isize i8 i16 i32 i64 bigint biguint rational rational32 rational64 bigrational f32 f64 std use_serde try-from"
- name: Rustfmt
rust: 1.35.0
diff --git a/Cargo.toml b/Cargo.toml
index 5c80a4c..b982bff 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -69,6 +69,10 @@ f32 = []
f64 = []
si = []
std = ["num-traits/std"]
+# The `try-from` feature provides `impl TryFrom<Time> for Duration` and `impl TryFrom<Duration> for
+# Time`. The `TryFrom` trait was stabilized in Rust 1.34.0 and will cause uom to fail to compile if
+# enabled with an earlier version of Rust.
+try-from = []
# The `use_serde` feature exists so that, in the future, other dependency features like num/serde
# can be added. However, num/serde is currently left out because it has not yet been updated to
# Serde 1.0. It is also necessary to name the feature something other than `serde` because of a
@@ -77,10 +81,6 @@ use_serde = ["serde"]
# Internal features to include appropriate num-* crates.
rational-support = ["num-rational"]
bigint-support = ["num-bigint", "num-rational/bigint"]
-# The `try-from` feature currently provides `impl TryFrom<Time> for Duration` and
-# `impl TryFrom<Duration> for Time`. These traits were stabilized in Rust 1.34.0, so this feature
-# will not work on earlier Rust versions.
-try-from = []
[[example]]
name = "base"
diff --git a/src/si/time.rs b/src/si/time.rs
index 6c00cd2..ab6aea7 100644
--- a/src/si/time.rs
+++ b/src/si/time.rs
@@ -1,5 +1,8 @@
//! Time (base unit second, s).
+use lib::time::Duration;
+use num::{AsPrimitive, FromPrimitive, Zero};
+
quantity! {
/// Time (base unit second, s).
quantity: Time; "time";
@@ -45,86 +48,83 @@ quantity! {
}
}
+/// An error encountered converting between `Time` and `Duration`.
+#[cfg(feature = "try-from")]
+#[derive(Debug, Clone, Copy)]
+pub enum TryFromError {
+ /// The given time interval was negative, making conversion to a duration nonsensical.
+ ///
+ /// To convert a negative time interval to a duration, first use `abs` to make it positive.
+ NegativeDuration,
+
+ /// The given time interval exceeded the maximum size of a `Duration`.
+ Overflow,
+}
-/// `Time` ⇌ `Duration` conversions.
+/// Attempt to convert the given `Time` to a `Duration`.
+///
+/// For possible failure modes, see [`TryFromError`][TryFromError].
+///
+/// ## Notes
+///
+/// The `Duration` to `Time` conversion is tested to be accurate to within 1 nanosecond (to allow
+/// for floating point rounding error). If greater precision is needed, consider using a different
+/// underlying storage type or avoiding the conversion altogether.
+///
+/// [TryFromError]: enum.TryFromError.html
#[cfg(feature = "try-from")]
-pub mod convert {
- use lib::time::Duration;
- use num::{AsPrimitive, FromPrimitive, Zero};
- use super::{nanosecond, second, Time};
-
- /// An error encountered in converting a `Time` to a `Duration`.
- #[derive(Debug, Clone, Copy)]
- pub enum TryFromError {
- /// The given time interval was negative, making conversion to a duration nonsensical.
- ///
- /// To convert a negative time interval to a duration, first use `abs` to make it positive.
- NegativeDuration,
- /// The given time interval exceeded the maximum size of a `Duration`.
- Overflow,
+impl<U, V> ::lib::convert::TryFrom<Time<U, V>> for Duration
+where
+ U: ::si::Units<V> + ?Sized,
+ V: ::num::Num + ::Conversion<V> + ::lib::cmp::PartialOrd + AsPrimitive<u64> + AsPrimitive<u32>,
+ second: ::Conversion<V, T = V::T>,
+ nanosecond: ::Conversion<V, T = V::T>,
+{
+ type Error = TryFromError;
+
+ fn try_from(time: Time<U, V>) -> Result<Self, Self::Error> {
+ if time < Time::<U, V>::zero() {
+ return Err(TryFromError::NegativeDuration);
+ }
+
+ let secs = time.get::<second>().as_();
+ let nanos = (time % Time::<U, V>::new::<second>(V::one())).get::<nanosecond>().as_();
+
+ Ok(Duration::new(secs, nanos))
}
+}
+
+/// Add documentation.
+#[cfg(feature = "try-from")]
+impl<U, V> ::lib::convert::TryFrom<Duration> for Time<U, V>
+where
+ U: ::si::Units<V> + ?Sized,
+ V: ::num::Num + ::Conversion<V> + FromPrimitive,
+ second: ::Conversion<V, T = V::T>,
+ nanosecond: ::Conversion<V, T = V::T>,
+{
+ type Error = TryFromError;
- /// Attempts to convert the given `Time` to a `Duration`.
+ /// Attempt to convert the given `Duration` to a `Time`.
///
/// For possible failure modes, see [`TryFromError`][TryFromError].
///
/// ## Notes
///
- /// The `Duration` to `Time` conversion is tested to be accurate to within 1 nanosecond
- /// (to allow for floating point rounding error). If greater precision is needed, consider
- /// using a different backing storage type or avoiding the conversion altogether.
+ /// The `Duration` to `Time` conversion is tested to be accurate to within 100 nanoseconds (to
+ /// allow for floating point rounding error). If greater precision is needed, consider using a
+ /// different underlying storage type or avoiding the conversion altogether.
///
/// [TryFromError]: enum.TryFromError.html
- impl<U, V> ::lib::convert::TryFrom<Time<U, V>> for Duration
- where
- U: ::si::Units<V> + ?Sized,
- V: ::num::Num + ::Conversion<V> + ::lib::cmp::PartialOrd + AsPrimitive<u64> + AsPrimitive<u32>,
- second: ::Conversion<V, T = V::T>,
- nanosecond: ::Conversion<V, T = V::T>,
- {
- type Error = TryFromError;
-
- fn try_from(time: Time<U, V>) -> Result<Self, Self::Error> {
- if time < Time::<U, V>::zero() {
- return Err(TryFromError::NegativeDuration);
- }
-
- let secs = time.get::<second>().as_();
- let nanos = (time % Time::<U, V>::new::<second>(V::one())).get::<nanosecond>().as_();
-
- Ok(Duration::new(secs, nanos))
- }
- }
-
- impl<U, V> ::lib::convert::TryFrom<Duration> for Time<U, V>
- where
- U: ::si::Units<V> + ?Sized,
- V: ::num::Num + ::Conversion<V> + FromPrimitive,
- second: ::Conversion<V, T = V::T>,
- nanosecond: ::Conversion<V, T = V::T>,
- {
- type Error = TryFromError;
- /// Attempts to convert the given `Duration` to a `Time`.
- ///
- /// For possible failure modes, see [`TryFromError`][TryFromError].
- ///
- /// ## Notes
- ///
- /// The `Duration` to `Time` conversion is tested to be accurate to within 100 nanoseconds
- /// (to allow for floating point rounding error). If greater precision is needed, consider
- /// using a different backing storage type or avoiding the conversion altogether.
- ///
- /// [TryFromError]: enum.TryFromError.html
- fn try_from(duration: Duration) -> Result<Self, Self::Error> {
- let secs = V::from_u64(duration.as_secs());
- let nanos = V::from_u32(duration.subsec_micros());
-
- match (secs, nanos) {
- (Some(secs), Some(nanos)) => {
- Ok(Time::<U, V>::new::<second>(secs) + Time::<U, V>::new::<nanosecond>(nanos))
- },
- _ => Err(TryFromError::Overflow),
+ fn try_from(duration: Duration) -> Result<Self, Self::Error> {
+ let secs = V::from_u64(duration.as_secs());
+ let nanos = V::from_u32(duration.subsec_micros());
+
+ match (secs, nanos) {
+ (Some(secs), Some(nanos)) => {
+ Ok(Time::<U, V>::new::<second>(secs) + Time::<U, V>::new::<nanosecond>(nanos))
}
+ _ => Err(TryFromError::Overflow),
}
}
}
@@ -154,6 +154,7 @@ mod tests {
if *v < V::zero() {
return TestResult::discard();
}
+
test_try_into(Time::try_from(Duration::from_nanos(v.as_())), v)
}
@@ -171,22 +172,26 @@ mod tests {
(Ok(t), true) => {
let d = Duration::from_nanos(v.as_());
let r = if d > t { d - t } else { t - d };
+
Duration::from_nanos(1) >= r
},
(Err(_), false) => true,
_ => false,
};
+
TestResult::from_bool(ok)
}
-
+
fn test_try_into(t: Result<Time<V>, TryFromError>, v: A<V>) -> TestResult {
let ok = if let Ok(t) = t {
let b = Time::new::<nanosecond>(v.as_());
let d = if t > b { t - b } else { b - t };
+
d <= Time::new::<nanosecond>(V::from_u8(100).unwrap())
} else {
false
};
+
TestResult::from_bool(ok)
}
}
diff --git a/tests/edition_check/Cargo.toml b/tests/edition_check/Cargo.toml
index 81182b2..f043ba9 100644
--- a/tests/edition_check/Cargo.toml
+++ b/tests/edition_check/Cargo.toml
@@ -9,3 +9,4 @@ uom = { path = "../.." }
[features]
default = []
use_serde = ["uom/use_serde"]
+try-from = ["uom/try-from"]
diff --git a/tests/feature_check/Cargo.toml b/tests/feature_check/Cargo.toml
index 803a6a5..e86934e 100644
--- a/tests/feature_check/Cargo.toml
+++ b/tests/feature_check/Cargo.toml
@@ -8,3 +8,4 @@ uom = { path = "../.." }
[features]
default = []
use_serde = ["uom/use_serde"]
+try-from = ["uom/try-from"] |
|
|
@Aehmlo, how are these changes coming? |
|
I’ve had a busy couple of weeks, but I anticipate getting this wrapped up this week or maybe over the coming weekend. |
|
Great! I'm coming off a busy long weekend myself. |
|
Okay, this week for sure. :) |
|
I started to take out |
|
Go ahead and leave the mod for the moment. |
|
@Aehmlo if you want to push the changes you've been able to make so far I hope to have time over the weekend to review the progress. Don't worry if it's not complete. |
|
I made the changes from the last review and found out there are errors for |
|
Apologies for basically disappearing; I’ll take a look over the weekend. |
|
Manually marking complete. Github only completed #131 even though both were referenced in the commit message. |
This PR changes @dunmatt's work from
From<Time>toTryFrom<Time>. I don't see any other bad inputs thatuomhas to handle explicitly itself except for a negative value; let me know if there's some other error case to consider.The glaringly obvious omission here is a version check:
TryFrom/TryIntoonly exists on stable in Rust 1.35 and later, but the minimum version is 1.28. I don't see anywhere else in the crate that deals with this (though I could've sworn there was something), but I wanted to see how you'd like to handle that. It could easily be an opt-in feature, with the burden of version correctness falling on the user, but maybe we want to use something else? I'm not sure abuild.rswould be great to introduce for just this, but I'm not really sure what else exists, so here's me soliciting ideas.