Skip to content

Added QuickCheck property to Interval#15

Open
uwaces wants to merge 5 commits intodevelopfrom
quick-check
Open

Added QuickCheck property to Interval#15
uwaces wants to merge 5 commits intodevelopfrom
quick-check

Conversation

@uwaces
Copy link
Collaborator

@uwaces uwaces commented Aug 26, 2020

This commit includes only one very basic property for intervals
at the time of the commit the property successfully runs for 100
instances.

This commit includes only one very basic property for intervals
at the time of the commit the property successfully runs for 100
instances.
@uwaces uwaces changed the title Added QuickCheck instances and property to IQuality and Interval Added QuickCheck property to Interval Aug 26, 2020
Copy link
Collaborator

@ChrisEPhifer ChrisEPhifer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! One place where there were (I think) redundant parentheses.

Also, we shouldn't merge this into master just yet; should hit develop, first, and only merge develop into master once we have something resembling a release.

Not sure if there's an easy way to update a PR to change the target branch, though...

EDIT: Just discovered that it's in fact very easy to change the merge target in a PR, and did so :) So disregard that part of this comment.

@ChrisEPhifer ChrisEPhifer changed the base branch from master to develop August 26, 2020 16:43
@ChrisEPhifer ChrisEPhifer added the enhancement New feature or request label Aug 26, 2020
@uwaces
Copy link
Collaborator Author

uwaces commented Aug 26, 2020

@ChrisEPhifer
I'm happy for this to be merged if you'd like, or we can wait for your modifications to Interval to be on develop, integrate the changes and then merge.

I think the integration work is boils down to changing the Arbitrary instance for Interval, just using the smart constructor you are creating (I left a TODO which should mean all that is required is un-commenting the code in the TODO).

Should this be merged? Or do you want to wait.

@ChrisEPhifer
Copy link
Collaborator

@ChrisEPhifer
I'm happy for this to be merged if you'd like, or we can wait for your modifications to Interval to be on develop, integrate the changes and then merge.

I think the integration work is boils down to changing the Arbitrary instance for Interval, just using the smart constructor you are creating (I left a TODO which should mean all that is required is un-commenting the code in the TODO).

Should this be merged? Or do you want to wait.

I think this should wait until #18 is merged, since if we don't wait it will just turn into another PR for the same work.

@ChrisEPhifer
Copy link
Collaborator

ChrisEPhifer commented Aug 27, 2020

@uwaces , #18 is now merged into develop, if you want to merge that into your quickcheck stuff and adapt to the new type of intervalFrom.

@ChrisEPhifer ChrisEPhifer mentioned this pull request Sep 21, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants