Skip to content
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.

Sign up for GitHub

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

Added an implementation of From<Time> for Duration #131

Closed
wants to merge 9 commits into from
Closed

Added an implementation of From<Time> for Duration #131

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5bb09d5
Added an implementation of From<Time> for Duration.
May 1, 2019
258ecb5
added a test for From
May 1, 2019
7515865
added a call to abs since Durations cannot be negative
May 1, 2019
14bcc35
wat wat wat wat wat wat wat
May 3, 2019
9755822
nightmare complete, the test passes
May 3, 2019
3e3969f
added another test case to fix code coverage
May 3, 2019
9caa47b
widened the acceptable range on the new test
May 3, 2019
82d6e99
added another test case to maybe fix code coverage?
May 3, 2019
8493f01
refactored from to remove else to hopefully fix code coverage
May 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/si/time.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,17 +55,18 @@ where
nanosecond: ::Conversion<V, T = V::T>,
{
fn from(t: Time<U, V>) -> Duration {
let mut secs = t.get::<second>();
if secs < V::zero() {
secs = V::zero() - secs;
}
let secs = secs.as_();
let t = if t < Time::<U, V>::new::<second>(V::zero()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why this is here, but do we want to silently convert negative times to (inherently positive) durations? I suppose this might be a matter of "time vs. time interval."

Copy link
Author

@dunmatt dunmatt May 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's really a silent conversion, the user has to manually call into in a context where the conversion is into an inherently positive type. I guess it's a question of which is least surprising:

  1. Absolute Value
  2. Wrapping Around
  3. Minimum 0
  4. panic!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that I would prefer panicking, although I’d imagine that others would see it differently.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? I just put that option there for completeness, From must not fail

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the context of that documentation page, "fallible functions" seem to be loosely be "functions that return Result<T, E> or Option<T>:

When constructing a function that is capable of failing the return type will generally be of the form Result<T, E>.

As such, I read that note as simply meaning "From does not return an error type." A more general argument for this interpretation is that it's almost nonsensical to say "this code must not panic" (with a couple of notable exceptions) in Safe Rust.

Panics signal an irrecoverable error, something that makes no sense and for which there's no sensible alternative route. In my mind, converting a negative time to a Duration is such an error, and it makes sense to panic, as this likely signals a logic error. However, panicking from within a library crate is certainly not ideal.

I actually might argue, if possible, that we should use TryFrom instead of From.

Copy link
Author

@dunmatt dunmatt May 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So TryFrom vs From is a different issue, there's certainly an argument to be made there. The trick is, doing that sort of error management (while certainly more rigorous) is enough of an ergonomics hit that if that's the direction uom decides to go I'm going to just continue defining my own conversion functions where and as I need them. I'm not saying you're wrong, you're not, TryFrom is definitely a more correct fit (even setting aside the absolute value question, NaN is sufficient to justify TryFrom), it just feels cumbersome (and the implementation is going to be even more of a nightmare).

So with that out of the way, I think your reading of the note is wrong, and here's why. There's absolutely no point saying "From does not return an error type", because there is no flexibility in from's return type. There's simply no choice what you return. It would be warning us to make sure our code builds, or warning us to not write bugs into our code.

If you assume they added (and emphasized) that warning for a reason, there must be more to it than that. It must be more general than "don't do what you can't do". The reason they warn us not to let it fail is because From is integral to Rust's failure handling mechanisms, and it's not at all clear what should happen when failure handling fails.

Copy link
Author

@dunmatt dunmatt May 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... so... I think I just accidentally convinced myself that this PR should be rejected on account of NaN and the infinities. sigh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to beat a dead horse, but I still believe that the meaning for that documentation for From is simply "the return value is not an error type." This is especially supported by the fact that you can impl From<A> for Option<B> to mimic TryFrom (and so there is indeed some flexibility in the return type). Regardless, I do think that TryFrom is the better fit here. May I ask why From is necessary for your use case/why TryFrom is insufficient?

Copy link
Author

@dunmatt dunmatt May 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that the trait is insufficient on its own (I mean, it's a bit cumbersome to have to do yet more error handling, but at the end of the day that's not that big a deal), it's more that the difficulty of making the change is greater than the amount I care about it being made. I've already sunk 100x more time into this PR than its actually worth to me, and if I were to double that investment I still don't think that's enough to adequately implement TryFrom.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, thanks. I think I might try to make up a TryFrom PR, fwiw. Thanks for your efforts on this.

Time::<U, V>::new::<second>(V::zero()) - t
} else {
t
};
let secs = t.get::<second>().as_();
let nanos = (t % Time::<U, V>::new::<second>(V::one())).get::<nanosecond>().as_();
Duration::new(secs, nanos)
}
}

#[cfg(all(test, feature = "std"))]
#[cfg(test)]
mod tests {
storage_types! {
types: Float;
Expand All @@ -76,11 +77,10 @@ mod tests {

#[test]
fn from() {
// let t = Time::new::<second>(21.5);
let t = Time::new::<second>(-21.5);
let d: Duration = t.into();
assert_eq!(21, d.as_secs());
// assert_eq!(5e8 as u32, d.subsec_nanos());
assert_eq!(d.as_secs(), 21);
assert!(499_999_999 <= d.subsec_nanos() && d.subsec_nanos() <= 500_000_001);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that we can't require this to be exactly 500,000,000?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precision loss when I subtract t from zero is my current best guess. The way I implemented it originally, by casting the time to f64 and then converting that to a Duration did allow this to be 500,000,000 exactly. Perhaps it's only the f32 test that loses precision?

}
}
}