Impl Arbitrary for Naive{Date,Time,DateTime} #848
Conversation
|
What is the value of adding unit tests here? The code seems relatively simple. |
| fn arbitrary(u: &mut Unstructured) -> arbitrary::Result<NaiveTime> { | ||
| let secs = u.int_in_range(0..=86_399)?; | ||
| let nano = u.int_in_range(0..=1_999_999_999)?; | ||
| let time = NaiveTime::from_num_seconds_from_midnight_opt(secs, nano) |
There was a problem hiding this comment.
What's the rationale for panicking here vs returning an error?
There was a problem hiding this comment.
My primary motivation was this comment #559 (comment), which you thumbed up.
I am OK with both approaches, both have their pros and cons. With this one (unwrap/expect) we'll detect the problem faster if somethings gets broken. (Arbitrary is implement that way, that if something returns an error, it retries to generate data until it succeeds).
I have this habit of adding them to almost every change I do. If you think they are not necessary I can drop them. Please let me know.
I am fine. Should I open a similar PR to P.S. I would be happy to implement all other stuff for the full support of Arbitrary and see it's released ASAP (in this regard 0.4.x is even more attractive for me at the moment). |
|
Just retarget this PR to 0.4.x, changes that get merged there eventually are merged into main. |
I'd love to see #559 moving forward, but looks like @asayers has no time to address the code review right now.
Since I am very interested in the support of
ArbitraryI've decided to cherry pick his commit and address the code review.What is in this PR:
Arbitrarybehind feature flag forNaiveDate,NaiveTime,NaiveDateTime.NaiveDateandNaiveTime(no unit test forNaiveDateTime, since it's implementation ofArbitraryis derived)arbitrarymodules (in the same fashion like it's done withserde)Considerations
Despite
does not make large difference when constructing
NaiveDateas @djc mentioned, I've decided to useYearFlags, so the both implementations forNaiveDateandNaiveTimeare implemented in similar infallible fashion.