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.

Sign up for GitHub

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

Jump to bottom

Move arbitrary instance of StateT to laws #1584

Merged
merged 5 commits into from
Apr 19, 2017
Merged

Move arbitrary instance of StateT to laws #1584

merged 5 commits into from
Apr 19, 2017

Conversation

kailuowang
Copy link
Contributor

This should be the only data type missing Arbitrary instance in laws.
fixes #1274

@kailuowang kailuowang added this to the cats-1.0.0 milestone Apr 3, 2017
@codecov-io
Copy link

codecov-io commented Apr 3, 2017 •
edited
Loading

Codecov Report

Merging #1584 into master will increase coverage by 0.71%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1584      +/-   ##
==========================================
+ Coverage    92.4%   93.11%   +0.71%     
==========================================
  Files         248      250       +2     
  Lines        3961     3992      +31     
  Branches      140      136       -4     
==========================================
+ Hits         3660     3717      +57     
+ Misses        301      275      -26
Impacted Files Coverage Δ
core/src/main/scala/cats/data/package.scala 83.33% <ø> (ø) ⬆️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala 90.32% <100%> (+0.49%) ⬆️
core/src/main/scala/cats/FlatMap.scala 92.3% <0%> (-7.7%) ⬇️
...aws/src/main/scala/cats/laws/ApplicativeLaws.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/instances/tuple.scala 100% <0%> (ø) ⬆️
.../src/main/scala/cats/syntax/applicativeError.scala 100% <0%> (ø) ⬆️
laws/src/main/scala/cats/laws/FlatMapLaws.scala 100% <0%> (ø) ⬆️
.../scala/cats/laws/discipline/ApplicativeTests.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/syntax/applicative.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/syntax/list.scala 100% <0%> (ø) ⬆️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 388acd1...69a6413. Read the comment docs.

@peterneyens
Copy link
Collaborator

As we are providing Arbitrary instances for Writer and State, should we also provide one for Reader?

@kailuowang
Copy link
Contributor Author

good catch @peterneyens , added.

@kailuowang
Copy link
Contributor Author

I added an Arbitrary for reader Reader by simply delegating to arbitraryForKleisli, but there is not tests for Reader to use it, hence the coverage miss. I think that should be okay?

@peterneyens peterneyens changed the title move arbitray instance of StateT to laws Move arbitrary instance of StateT to laws Apr 7, 2017
@ceedubs
Copy link
Contributor

ceedubs commented Apr 17, 2017 •
edited
Loading

@kailuowang I'm hesitant to add it without any tests that use it. Especially because this is the sort of thing that can easily lead to ambiguous instances. I'd be inclined to add a test and even see if we could get by without the instances that are specific to Id (though I suspect that implicit search won't be that kind to us).

On a separate note, our instances for function-like types like Kleisli should probably be using Cogen so they don't just ignore the input and return a static value. But that is probably outside of the scope of this PR. I've created #1605

ceedubs added a commit to ceedubs/cats that referenced this pull request Apr 17, 2017
@ceedubs
Use Cogen for Kleisli and Func Arbitrary instances
67a8a81 
I think that this resolves typelevel#1605. It may have merge conflicts with typelevel#1584
and I think that one should probably be merged first.

I noticed a slight discrepancy between how I've implemented this (taking
implicit `Cogen` and `Arbitrary` instances) vs how the current `StateT`
arbitrary instance works (it takes an implicit `Arbitrary[S => F[(S, A)]]]). I'm not really sure whether one approach is better than the other. We should probably align on one of the two, though.
@ceedubs ceedubs mentioned this pull request Apr 17, 2017
Closed
@kailuowang
Copy link
Contributor Author

@ceedubs I added a test for Reader to hit the arbitrary instance created for it. I also verified that we can't get by without this instance specific to Id, i.e. removing the instance failed the compilation of the test.

ceedubs
ceedubs reviewed Apr 17, 2017
View reviewed changes
tests/src/test/scala/cats/tests/KleisliTests.scala Outdated
@@ -15,6 +15,9 @@ class KleisliTests extends CatsSuite {
implicit def kleisliEq[F[_], A, B](implicit A: Arbitrary[A], FB: Eq[F[B]]): Eq[Kleisli[F, A, B]] =
Eq.by[Kleisli[F, A, B], A => F[B]](_.run)

implicit def readerEq[A, B](implicit A: Arbitrary[A], FB: Eq[Id[B]]): Eq[Reader[A, B]] =
Eq.by[Reader[A, B], A => Id[B]](_.run)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but this could probably just delegate to the kleisliEq couldn't it?

@ceedubs
Copy link
Contributor

ceedubs commented Apr 17, 2017

@peterneyens do you want to take another look after the most recent changes?

peterneyens
peterneyens approved these changes Apr 19, 2017
View reviewed changes
Copy link
Collaborator

@peterneyens peterneyens left a comment

Choose a reason for hiding this comment

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

I was about to add a similar Reader test in #1618.

@peterneyens peterneyens merged commit 4e381d2 into typelevel:master Apr 19, 2017
@stew stew removed the in progress label Apr 19, 2017
@kailuowang kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ceedubs ceedubs ceedubs approved these changes

@peterneyens peterneyens peterneyens approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.0-MF
Development

Successfully merging this pull request may close these issues.

Add Arbitrary instances for all data types
5 participants
@kailuowang @codecov-io @peterneyens @ceedubs @stew