Conversation
ulysses4ever
left a comment
There was a problem hiding this comment.
Thanks! And sorry for a long wait.
I think it's mergeable although I'd make different choices sometimes.
First, reviewing commit-by-commit on GitHub is hard. While it is true that reading your changes commit-by-commit makes more sense (and thank you for suggesting it), commenting those changes is another story. Comments you leave under a certain commit (as opposed to in the overall "Files changed" tab of a PR) are second-class citizens in certain ways making them really hard to track. So, I'd much prefer you did the mappend-><> substitution in a separate PR (I know it may make little sense on its own).
Second, my main pain point here is in textual changes: sometimes it feels like the new text spells out "monoid and semigroup" where just "monoid" would suffice, and this makes it harder to read. For instance,
It's possible to make instances of
SemigroupandMonoidthat don't follow these rules,
I think it could do with just "instances of Monoid". Especially given that just above it does say talk only about Monoids ("Before moving on to specific instances of Monoid"), and that's the style I 'd prefer because every time use say something about monoids this usually implies things about semigroups and unless there's a vbery particular need for mentioning semigoroups, I'd prefer to just read about monoids as a whole. But I understand that it's a stylistic choice, so I don't want to argue about it...
On the other hand, where old text talks about code specifically, I feel, it should mention both instances instead of one. E.g.
Here's the
Monoidinstance:
instance Semigroup (DiffList a) where
DiffList f <> DiffList g = DiffList (\xs -> f (g xs))
instance Monoid (DiffList a) where
mempty = DiffList (\xs -> [] ++ xs)
(DiffList f) `mappend` (DiffList g) = DiffList (\xs -> f (g xs))
This doesn't make sense: it says "here's one instance" and shows two. It must say that two instancces are coming.
|
Good to know reviewing individual commits on here is that annoying.
I have extracted the first two commits into #154.
The main reason I chose to mention |
9a512ca to
4661270
Compare
MatthijsBlom
left a comment
There was a problem hiding this comment.
I rebased onto #154. Hopefully that will improve the diff when that PR is merged.
Feel free to nitpick.
After everything is settled I plan to squash into sensible chunks.
4661270 to
5873523
Compare
Signed-off-by: Matthijs <[email protected]>
ulysses4ever
left a comment
There was a problem hiding this comment.
Thanks! A tiny bit of ideas inline but it's already quite good!
| By noticing and writing down these properties, we have chanced upon *monoids*! | ||
| A monoid is when you have an associative binary function and a value which acts as an identity with respect to that function. | ||
| By noticing and writing down these properties, we have chanced upon *semigroups* and *monoids*! | ||
| A semigroup is when you have an associative binary function, and a monoid is when you also have a value which acts as an identity with respect to that function. |
There was a problem hiding this comment.
| A semigroup is when you have an associative binary function, and a monoid is when you also have a value which acts as an identity with respect to that function. | |
| A semigroup is when you have an associative binary function, and a monoid is when you have a semigroup and a value that acts as an identity with respect to that function. |
There was a problem hiding this comment.
Having just "also" there doesn't pull the weight I feel. It's important to be crystal clear about this hierarchy.
There was a problem hiding this comment.
The "that function" reference feels broken, that way.
Maybe a total rewrite?
| A semigroup is when you have an associative binary function, and a monoid is when you also have a value which acts as an identity with respect to that function. | |
| When you have an associative binary function, we call that a semigroup. | |
| And if you also have a value that acts as an identity with respect to that function, then we call it a monoid instead. |
( The 'instead' is there to make clear that it is the whole that is called the 'monoid', not just the identity element by itself. )
| There are a lot of other monoids to be found in the world of Haskell, which is why the `Monoid` type class exists. | ||
| It's for types which can act like monoids. | ||
| Let's see how the type class is defined: | ||
| There are a lot of other monoids and semigroups to be found in the world of Haskell, which is why the `Monoid` and `Semigroup` type classes exist. |
There was a problem hiding this comment.
| There are a lot of other monoids and semigroups to be found in the world of Haskell, which is why the `Monoid` and `Semigroup` type classes exist. | |
| There are a lot of other monoids and semigroups to be found in the world of Haskell, which is why the respective type classes (`Semigroup` and `Monoid`) exist. |
would read a little cleaner I feel.
There was a problem hiding this comment.
I would agree, if this weren't the first time these classes were named/mentioned.
| It's for types which can act like monoids. | ||
| Let's see how the type class is defined: | ||
| There are a lot of other monoids and semigroups to be found in the world of Haskell, which is why the `Monoid` and `Semigroup` type classes exist. | ||
| It's for types which can act like monoids and semigroups. |
There was a problem hiding this comment.
The ordering in "monoids and semigroups" is a bit confusing, I feel, because semigroups come first in the sense that any monoid is a semigroup as well. But maybe it's not a big deal. Just in case, I add a suggestion but it's up to you whether to acccept it.
| It's for types which can act like monoids and semigroups. | |
| It's for types which can act like semigroups and monoids. |
There was a problem hiding this comment.
I do not expect the reader's intuition to be clear on this yet, at this point in the text, so I do not expect a problem.
If these two words are to be swapped, I think the preceding two occurrences should be swapped as well, to preserve the 'rhythm'.
Currently the 'most important' of the two comes first, in these two sentences, somewhat in agreement with the title of this section.
This reverts commit d2e4594.
Fix #49
I recommend reviewing the commits individually, in order.
As some changes' contexts are bigger than GitHub's UI suggests, consider reviewing locally.