Skip to content
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

Upgrade to quickcheck 1.0 #302

Closed
jhpratt opened this issue Jan 9, 2021 · 4 comments · Fixed by #304
Closed

Upgrade to quickcheck 1.0 #302

jhpratt opened this issue Jan 9, 2021 · 4 comments · Fixed by #304
Labels
A-third-party Area: implementations of traits from other crates
Milestone

Comments

@jhpratt
Copy link
Member

jhpratt commented Jan 9, 2021

The time crate should upgrade the quickcheck dependency to 1.0 prior to time's 0.3 release. The API is largely similar, but there are some changes. Notably, it is no longer possible (for the time, see BurntSushi/quickcheck#267) to generate a value within a given range. This can be worked around with the following function. It has some limitations with regard to overflow within the type, but I don't believe that should be an issue for any values used in the time crate.

fn arbitrary_between<T>(g: &mut Gen, min: T, max: T) -> T
where
    T: PartialOrd
        + AddAssign
        + Add<Output = T>
        + Sub<Output = T>
        + Rem<Output = T>
        + Arbitrary
        + Copy,
{
    #[allow(clippy::eq_op)]
    let zero = min - min;

    let range = max - min;
    let mut within_range = T::arbitrary(g) % range;

    if within_range < zero {
        within_range += range;
    }

    within_range + min
}

One thing I noticed in the documentation that I didn't before (I'm not sure if it was present or not and can't be bothered to check) is that the Gen::size() method does not need to be used, as all types in the time crate have a constant size; the example used as a use case is Vec, which most certainly does not. As such, the entire foo_respects_generator_size and test_generator_size! bit can be dropped, I believe.

This should likely be done in coordination with #299, though it's not strictly necessary. That change by itself will be nearly trivial.

cc @emlun if you're interested with your macro magic. No big deal if not.

@jhpratt jhpratt added the A-third-party Area: implementations of traits from other crates label Jan 9, 2021
@emlun
Copy link
Contributor

emlun commented Jan 11, 2021

I'll take a look!

@jhpratt
Copy link
Member Author

jhpratt commented Jan 11, 2021

Whoops, forgot to leave a second comment! I have it pretty much all figured out, as I stripped out the bit mentioned in the original issue.

I believe I found a bug in quickcheck, but have yet to file an issue on it. Other than that I think everything works.

Thank you, though!

@emlun
Copy link
Contributor

emlun commented Jan 11, 2021

Oh, okay. 🙂 In that case I'd gladly review the changes instead, if you want. Let me know if you need anything!

@jhpratt
Copy link
Member Author

jhpratt commented Jan 11, 2021

Ok! I'll let you know when a PR goes up if you'd like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-third-party Area: implementations of traits from other crates
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants