Optionally implement Arbitrary for Naive{Date,Time,DateTime} #559
Conversation
|
It seems like |
|
@quodlibetor I'd love to use it.
This test would be much more smaller and, what is also important, not biased (unlike me, a developer). |
|
If someone is willing to rebase or resubmit it, I would certainly consider it worthwhile. |
|
@djc Hi! The PR was rebased (unfortunately I also missed that). |
| #[cfg(feature = "arbitrary")] | ||
| impl arbitrary::Arbitrary<'_> for NaiveTime { | ||
| fn arbitrary(u: &mut arbitrary::Unstructured) -> arbitrary::Result<NaiveTime> { | ||
| let secs = u.int_in_range(0..=86_400)?; |
There was a problem hiding this comment.
If I understand correctly these ranges should not be inclusive... a day has up to 86399 seconds, and frac is supposed to be smaller than 2_000_000_000.
See
Line 391 in 1281ce0
| impl arbitrary::Arbitrary<'_> for NaiveDate { | ||
| fn arbitrary(u: &mut arbitrary::Unstructured) -> arbitrary::Result<NaiveDate> { | ||
| let year = u.int_in_range(MIN_YEAR..=MAX_YEAR)?; | ||
| let ord = u.int_in_range(1..=366)?; |
There was a problem hiding this comment.
Could we construct the YearFlags and determine the exact ndays in the year so constructing the date wouldn't fail
There was a problem hiding this comment.
That doesn't seem like it makes a particularly large difference?
| fn arbitrary(u: &mut arbitrary::Unstructured) -> arbitrary::Result<NaiveTime> { | ||
| let secs = u.int_in_range(0..=86_400)?; | ||
| let frac = u.int_in_range(0..=2_000_000_000)?; | ||
| Ok(NaiveTime { secs, frac }) |
There was a problem hiding this comment.
This feels like something that might be better to use from_num_seconds_from_midnight_opt (and unwrap it with a clear error that it is a chrono bug if it fails). That way you can't silently make invalid NaiveTimes.
And yeah, a secs of 86_400 will be rejected.
I'm not sure if you guys are interested in taking a change like this. If you are, I'd be happy to fill in more impls.