Add Applicative for Arrow #2078
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2078 +/- ##
==========================================
+ Coverage 94.66% 94.67% +<.01%
==========================================
Files 318 318
Lines 5383 5388 +5
Branches 207 216 +9
==========================================
+ Hits 5096 5101 +5
Misses 287 287
Continue to review full report at Codecov.
|
|
This is cool. I start to think def pure[B](b: B): F[A, B] = lift(_ => b) Also we should probably move it to Applicative companion object, having them in Arrow companion won't make it available for implicit search. |
| * res0: (Long, Int) = (3,6) | ||
| * }}} | ||
| */ | ||
| implicit def catsSemigroupalForArrow[F[_, _], A](implicit F: Arrow[F]): Semigroupal[F[A, ?]] = new Apply[F[A, ?]] { |
There was a problem hiding this comment.
it's probably worthwhile to override product.
There was a problem hiding this comment.
Good catch, will do.
|
You're totally right, that seems to work. We'll have to check the laws but just from intuition it seems right. I have heard |
|
In case you don’t know this paper, I think it’s a good read about the relationship between arrows and applicatives: http://homepages.inf.ed.ac.uk/slindley/papers/idioms-arrows-monads.pdf |
Also adds tests
|
Cool. Strengthened the instance to |
| * scala> g(5) | ||
| * res0: String = hello | ||
| * }}} | ||
| */ |
There was a problem hiding this comment.
Thanks for writing these doctests. But doctests in an instance are probably not necessary. They won't be included in apidoc, and these methods are already thoroughly tested in law tests of the instance. So, although the more tests the better, having them here has two slight cons, a) more code to read, in an instance definition, people want to quickly see how the instance is the implemented, rather than the API usage. and b) they provide redundant test coverage for these implementations, so if somehow the instance is not test by law tests, the test coverage report won't expose that.
There was a problem hiding this comment.
Ah, yeah, that makes sense.
Adds an
Applyinstance as discussed in #2069It can be weakened to
Semigroupalif we don't buy theApply.Currently, it is implicit. There was discussion in the issue on whether it should be explicit or implicit. Either answer seems reasonable so I wrote the doctest to be compatible with either way people end up preferring.
Do we need additional tests over the doctest?