Revamp Semigroup doc #1442
Revamp Semigroup doc #1442
Conversation
Current coverage is 92.22% (diff: 100%)@@ master #1442 diff @@
==========================================
Files 242 242
Lines 3615 3615
Methods 3546 3546
Messages 0 0
Branches 69 69
==========================================
+ Hits 3333 3334 +1
+ Misses 282 281 -1
Partials 0 0
|
| ``` | ||
|
|
||
| must be the same as | ||
| `combine` also needs to be associative, which means the following equality must hold for any choices of `x`, `y`, and |
There was a problem hiding this comment.
The flow feels odd here where it's just been declared emphatically that Semigroup is about an associative binary operation, then to immediately say that combine also needs to be associative.
Maybe something like:
where combine is the associative operation, which...
| You'll notice that instead of declaring `one` as `Some(1)`, I chose | ||
| `Option(1)`, and I added an explicit type declaration for `n`. This is | ||
| because there aren't type class instances for `Some` or `None`, but for | ||
| `Option`. If we try to use `Some` and `None`, we'll get errors: |
There was a problem hiding this comment.
I know it's detail, but I'm slightly loathe to lose this bit on Some/None, as it's something which I've seen trip up a lot of people knew to type classes.
There was a problem hiding this comment.
I think we're going to move these "gotchas" to a more generic location, being handled at this PR: #1404
| Monoid[Int].combine(Monoid[Int].empty, x) | ||
| ``` | ||
|
|
||
| # Exploiting laws: associativity and identity |
There was a problem hiding this comment.
I don't really see how this sub-heading fits with the text under it.
There was a problem hiding this comment.
Ah I had a different intent for this section initially but went with a different direction and forgot to change it, oops :-)
| `empty` method to semigroup's `combine`. The `empty` method must return a | ||
| value that when combined with any other instance of that type returns the | ||
| other instance, i.e. | ||
| `Monoid` extends the power of `Semigroup` by providing an additional `empty` value. |
There was a problem hiding this comment.
This reads much more cleanly. 👍
| ```tut:book | ||
| import cats.syntax.semigroup._ | ||
|
|
||
| 1 |+| 2 |
There was a problem hiding this comment.
Addition always seems like a poor motivating example for this syntax to me. Maybe Option[Int] might make the case better.
There was a problem hiding this comment.
The issue I had with introducing the Option[A] Semigroup this early was there are some nuances to it with regards to how it handles None. Since the Semigroup instance is sort of incidental because it really is a Monoid instance, I opted to introduce it in the Monoid section instead.
I could perhaps show an early Map example and say details to come later in this post. As I type that out I think that's a good idea, I'll go ahead and do that.
| ``` | ||
|
|
||
| Examples. | ||
| Many types that form a `Semigroup` also form a `Monoid`, such as `Int`s (with `0`) and `Strings` (with `""`). |
There was a problem hiding this comment.
Probably tangential to this PR, but I've read discussions on the variations of
T:
- has a
- is a
- forms a
Typeclass[T], maybe this is something nice to have a sentence about? I don't care which phrase is used though (maybe a note that they're saying the same thing), and I'm guessing somewhere on http://typelevel.org/cats/typeclasses.html would be appropriate.
There was a problem hiding this comment.
Yeah I ran into this same issue too. I'll keep that in mind on my next pass through (or you can make a PR! :D ) Thanks Long!
|
👍 LGTM. Definitely a big improvement from the previous version. |
No description provided.