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

Updates traversable to test the correct laws #61

Merged
merged 5 commits into from
Feb 23, 2022

Conversation

solomon-b
Copy link
Contributor

Resolves issue #24

I wasn't able to implement the Naturality test correctly, but I left it in as a comment in case someone else can get it working. I think it follows from Identity and Composition so we are okay without it.

Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! :)

I think it follows from Identity and Composition so we are okay without it.

Do you know a proof?

I'm fine with scrapping support for GHC 7.10 BTW, so you can use TypeApplications.

src/Test/QuickCheck/Classes.hs Show resolved Hide resolved
src/Test/QuickCheck/Classes.hs Outdated Show resolved Hide resolved
src/Test/QuickCheck/Classes.hs Outdated Show resolved Hide resolved
@sjakobi
Copy link
Member

sjakobi commented Feb 14, 2022

BTW, it would be nice to have tests for the sequenceA laws, too. Feel free to add them, but there's no obligation. :)

sjakobi added a commit that referenced this pull request Feb 16, 2022
This is motivated by #61 and #62.
sjakobi added a commit that referenced this pull request Feb 16, 2022
This is motivated by #61 and #62.
@solomon-b
Copy link
Contributor Author

I think it follows from Identity and Composition so we are okay without it.

Do you know a proof?

hmm, i actually haven't been able to find a proof, but this link claims it is guaranteed to hold:
https://en.wikibooks.org/wiki/Haskell/Traversable#The_Traversable_laws

Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

Thanks!

One more nit…

src/Test/QuickCheck/Classes.hs Outdated Show resolved Hide resolved
Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

Thanks! :)

Since this is a breaking change, I'll merge this after cutting the v0.5.7 release (#64).

@sjakobi sjakobi requested a review from conal February 17, 2022 00:21
@solomon-b
Copy link
Contributor Author

Great!

@sjakobi
Copy link
Member

sjakobi commented Feb 17, 2022

Regarding naturality, note this change: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7492

@solomon-b
Copy link
Contributor Author

Well there you go. Should we hold off on merging this until a working implementation is produced?

@sjakobi
Copy link
Member

sjakobi commented Feb 17, 2022

Well there you go. Should we hold off on merging this until a working implementation is produced?

When do you expect it to land?

IMHO the existing changes are good additions which shouldn't be held back for very long.

@solomon-b
Copy link
Contributor Author

Well there you go. Should we hold off on merging this until a working implementation is produced?

When do you expect it to land?

IMHO the existing changes are good additions which shouldn't be held back for very long.

To be honest, I'm a little stumped on this how to fix it. Is there anyone with more quickcheck experience who we can ask? The issue is the rank2 type.

@sjakobi sjakobi merged commit e19463f into haskell-checkers:master Feb 23, 2022
@sjakobi sjakobi mentioned this pull request Feb 23, 2022
@sjakobi
Copy link
Member

sjakobi commented Mar 8, 2022

Published in v0.6.0: https://hackage.haskell.org/package/checkers-0.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants