Add SubSecondRound trait #217
Conversation
src/round.rs
Outdated
| let span = span_for_digits(digits); | ||
| let rem = self.nanosecond() % span; | ||
| if rem > 0 { | ||
| let rev = span - rem; |
There was a problem hiding this comment.
what is rev meant to signify? "Should we reverse this computation"? Could you think of a way to clarify this variable to make this calculation a bit more obvious? Honestly I expected there to be a comparison to zero in here somewhere on my first read through.
Maybe?
let round_up = (span - rem) <= rem;
if round_up {
...
There was a problem hiding this comment.
In 74ec7aa I've renamed variables to delta_down and delta_up. Logicwise, I believe this is optimal, since delta_up is used for the up branch.
src/round.rs
Outdated
| pub trait SubSecondRound { | ||
| /// Return a copy rounded to the specified number of subsecond digits. With | ||
| /// 9 or more digits, self is returned unmodified. Halfway values are | ||
| /// rounded up (away from zero). |
There was a problem hiding this comment.
could you put in a doctest that includes
dt = Utc.ymd(2018, 1, 11).and_hms_milli(154);
assert_eq!(dt.subsecond_round(2).nanosecond(), 150_000_000);
assert_eq!(dt.subsecond_round(1).nanosecond(), 200_000_000);
The point being to include a small number of digits that are easily understandable.
src/round.rs
Outdated
| /// Return a copy rounded to the specified number of subsecond digits. With | ||
| /// 9 or more digits, self is returned unmodified. Halfway values are | ||
| /// rounded up (away from zero). | ||
| fn round(self, digits: u16) -> Self; |
There was a problem hiding this comment.
could you rename this to subsecond_round.
There was a problem hiding this comment.
Assuming your reason is to allow possibly other types of rounding in the future, I renamed to round_subsecs and trunc_subsecs The semi-abbreviated "subsecs" is used elsewhere in chrono, is a little shorter, and if other round_* later appear, they will sort together.
There was a problem hiding this comment.
Yup, that was why, and I was torn between the order of the words, so your choice is fine.
In this case could you rename the trait to SubsecRound, though?
There was a problem hiding this comment.
Renamed to SubsecRound.
src/round.rs
Outdated
|
|
||
| /// Return a copy truncated to the specified number of subsecond | ||
| /// digits. With 9 or more digits, self is returned unmodified. | ||
| fn trunc(self, digits: u16) -> Self; |
There was a problem hiding this comment.
could you rename this to subsecond_trunc
There was a problem hiding this comment.
Per above, renamed to trunc_subsecs
src/round.rs
Outdated
| fn round(self, digits: u16) -> Self; | ||
|
|
||
| /// Return a copy truncated to the specified number of subsecond | ||
| /// digits. With 9 or more digits, self is returned unmodified. |
There was a problem hiding this comment.
could you add a similar doctest for trunc? Same numbers as for round would be ideal.
src/lib.rs
Outdated
| @@ -418,6 +419,7 @@ pub mod prelude { | |||
| #[doc(no_inline)] pub use {NaiveDate, NaiveTime, NaiveDateTime}; | |||
| #[doc(no_inline)] pub use Date; | |||
| #[doc(no_inline)] pub use {DateTime, SecondsFormat}; | |||
| #[doc(no_inline)] pub use {SubSecondRound}; | |||
There was a problem hiding this comment.
There shouldn't be curlies around the SubSecondRound trait, since it's a single item.
src/round.rs
Outdated
| /// Return a copy rounded to the specified number of subsecond digits. With | ||
| /// 9 or more digits, self is returned unmodified. Halfway values are | ||
| /// rounded up (away from zero). | ||
| fn round(self, digits: u16) -> Self; |
There was a problem hiding this comment.
I believe subsecond_round and subsecond_trunc should both take &self, since they don't mutate self and TimeLike is not Copy.
There was a problem hiding this comment.
All internal implementations of TimeLike are Copy I believe. I don't think there is any way to avoid a copy in the implementation or otherwise make it more efficient with this. It might actually thwart compiler optimizations? Presumably that is why the Add and Sub implementations are also not by reference? I can be made to work if necessary however. See da4c19b using Clone, or 14504b2 using Copy. Let me know if you still think either of these are an improvement, and I'll hope to better understand the rationale.
There was a problem hiding this comment.
Hmm yeah the tests that you have demonstrate that they work as though they're copy. I wasn't concerned so much about efficiency as I was about a clear and easy-to-use API. So, specifically, if we weren't copy then the following two lines would fail with a use-after-move error:
dt.round_subsec(1);
dt.round_subsec(1);
But you have a bunch of tests that do that. I thought that it wouldn't work because e.g. DateTime<TimeZone> doesn't derive(Copy) so I'm not sure how consistency is being maintained right now.
There was a problem hiding this comment.
The lack of derive surprised me as well, but its actually implemented manually at datetime.rs#L437 and even tested. Note that use-after-move, if lacking Copy, also applies to chrono's Add and Sub:
let future = dt + Duration::nanoseconds(1);
let past = dt - Duration::nanoseconds(1);There was a problem hiding this comment.
Oh, ha, yup. Okay I'll give this one more once-over and probably merge tonight. Thanks!
|
Thanks! |
Explicit rounding/truncation of subseconds is a feature offered in the baked in time libraries of other languages like ruby and go. Rounding can be used to decrease the error variance when
displaying/serializing/persisting to lower precision. Truncation is the (implicit) default behavior in Chrono display formatting. Either can be used to guarantee equality (e.g. for testing) when round-tripping through a lower precision format, like PostgreSQL timestamps.
As in this implementation, its pretty easy to add support for this in chrono via an extension trait, implemented for
Timelike+Add+Sub. I don't see much downside to implementing this way vs baking it directly in to the Timelike structs?