Add contravariant instances for Const and Kleisli

[philwills]

I realise that this is very similar to @vikraman's work in #590, but I think this addresses the issues raised by @ceedubs and it was much easier to get this working against master than raising a PR onto that branch, thanks to the removal of ArbitraryK.

[vikraman]

Ah thanks, I was running into diverging implicits with [Kleisli[F, ?, A]] so I just gave up.

[ceedubs]

Am I correct to guess that you were forced into doing this because the implicit instances wasn't inferred?

[philwills]

No, I've just checked and it works fine without. I was just blindly following the pattern in the rest of the test suite. Happy to push up without if that's preferred.

[ceedubs]

I think if it can be inferred then we should probably let it be inferred in the tests. If nothing else, it provides an extra test that we don't break implicit resolution for this instance without realizing it :)

[philwills]

I've gone through and changed KleisliTests to not explicitly specify the implicits.

[codecov-io]

Current coverage is 76.22%

Merging #626 into master will increase coverage by +0.09% as of 366c55c

@@            master    #626   diff @@
======================================
  Files          159     159       
  Stmts         2070    2074     +4
  Branches        69      69       
  Methods          0       0       
======================================
+ Hit           1576    1581     +5
  Partial          0       0       
+ Missed         494     493     -1

Review entire Coverage Diff as of 366c55c

Powered by Codecov. Updated on successful CI builds.

[ceedubs]

@philwills Sorry for the hassle, but I think that while it was fine to not be explicit about the Contravariant instance, there is actually purpose in the other places doing what they are doing. It's subtle, but here's what's going on:

If you just type implicitly[FlatMap[Kleisli[Option, Int, ?]]] you will actually pick up the kleisliMonadReader instance since Option has a Monad instance and MonadReader is a subtype of FlatMap. These "explicit implicits" were being used to ensure that we actually tested the specific instance method that we were trying to test. This is why you may have noticed that codecov.io reported a drop in test coverage after your last commit even though it didn't seem like it should have.

In some places we are using a ListWrapper helper that doesn't have any implicit instances to help with this sort of thing. We could either use that in these tests or just revert it to what it was doing before and leave that effort for another PR.

In the case of the Contravariant instance, it's okay to not explicitly call the method, because there's only one method that returns a Contravariant instance and that's the one we want. Sorry, when I brought it up I didn't realize what was going on everywhere else or I would have clarified.

[ceedubs]

The issues with the tests are subtle, and hopefully at some point we form a good convention for this sort of thing. But other than that, the actual implementation looks correct and clean to me. Thanks for this, @philwills!

[philwills]

OK, I've reverted the change to all but the new test. Thanks for being so clear with your explanation.

[ceedubs]

Looks great! 👍

[non]

Looks good to me. Thanks @philwills ! 👍