-
Notifications
You must be signed in to change notification settings - Fork 546
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.
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
Convert FixedOffset::{east, west}
to return Result
#1468
Conversation
837d1ab
to
68d9e3e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 0.5.x #1468 +/- ##
==========================================
+ Coverage 94.33% 94.34% +0.01%
==========================================
Files 37 37
Lines 17004 17009 +5
==========================================
+ Hits 16040 16047 +7
+ Misses 964 962 -2 ☔ View full report in Codecov by Sentry. |
The spelling of the existing docs doesn't seems entirely correct English. I'll adjust that. |
let fixed_offset = FixedOffset::east(secs) | ||
.expect("Could not generate a valid chrono::FixedOffset. It looks like implementation of Arbitrary for FixedOffset is erroneous."); | ||
Ok(fixed_offset) | ||
Ok(FixedOffset::east(secs).unwrap()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the unwrapping be prevented by mapping the error? Untested: FixedOffset::east(secs).or(Err(arbitrary::Error))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The range we supply to arbitrary::Unstructured::int_in_range
is exactly equal to the range FixedOffset
supports, and I don't see that changing. This should simply never fail, so I even considered the expect
to be overkill.
} else { | ||
None | ||
pub const fn east(secs: i32) -> Result<FixedOffset, Error> { | ||
match -86_400 < secs && secs < 86_400 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I appreciate the conversion to match
, in the future I would appreciate it more in a separate commit. 👍
The easiest methods to convert yet!
Both
Error::InvalidArgument
andError::OutOfRange
could be reasonable return types.The real-world limit of offsets is currently 14 hours, which puts area's using it 2 hours to the other side of the international date line.
The type limit is generous and somewhat arbitrarily limited to be less than 24 hours. At some point it turned out limiting the offset to less than a day keeps the surrounding code saner.
Because the limit is artificial and specific to chrono I went with
Error::OutOfRange
.cc @Zomtir.