Convert timestamp methods on DateTime to return Results
#1495
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 0.5.x #1495 +/- ##
==========================================
- Coverage 94.11% 94.04% -0.08%
==========================================
Files 37 37
Lines 17125 17033 -92
==========================================
- Hits 16118 16019 -99
- Misses 1007 1014 +7 ☔ View full report in Codecov by Sentry. |
src/datetime/tests.rs
Outdated
| @@ -217,15 +219,15 @@ fn test_datetime_from_timestamp_nanos() { | |||
| let nanos = parsed.timestamp_nanos().unwrap(); | |||
| assert_eq!( | |||
| Some(DateTime::from_timestamp_nanos(nanos)), | |||
There was a problem hiding this comment.
I'd prefer to do the comparison in the Result domain:
assert_eq!(
Ok(DateTime::from_timestamp_nanos(nanos)),
DateTime::from_timestamp(nanos / A_BILLION, (nanos % A_BILLION) as u32)
);
src/datetime/mod.rs
Outdated
| @@ -282,7 +282,8 @@ impl<Tz: TimeZone> DateTime<Tz> { | |||
| subsec_nanos -= 1_000_000_000; | |||
| timestamp += 1; | |||
| } | |||
| try_opt!(timestamp.checked_mul(1_000_000_000)).checked_add(subsec_nanos) | |||
| let ts = try_err!(ok_or!(timestamp.checked_mul(1_000_000_000), Error::OutOfRange)); | |||
There was a problem hiding this comment.
If I remember correctly, we want the value to be unwrapped anyway in all instances. I sugges try_ok_or instead that directly unwraps T or returns the error early.
try_err!(ok_or!(..)) vs try_ok_or!
I don't really mind chaining macros, but wanted to have it mentioned.
pitdicker
force-pushed
the
convert_timestamp
branch
from
f07b60c to
d430631
Compare
|
Updated to have a |
|
Uh, what/where/why is the |
|
Sorry, just after your review. It is a combination of |
|
I don't like the |
pitdicker
force-pushed
the
convert_timestamp
branch
from
d430631 to
afee651
Compare
pitdicker
force-pushed
the
convert_timestamp
branch
from
afee651 to
cc5fbf1
Compare
Yea, that is better. |
All the preparations are in!
Also adds an
ok_or!macro.cc @Zomtir.