-
Notifications
You must be signed in to change notification settings - Fork 90
Expand on documentation #217
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.
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
Changes from all commits
9464e86
276a5e5
c48a6e6
e7de566
da12208
7f4e169
8157482
285f934
0b62cf3
6587f19
bf1a5b4
0fb3e86
0bfaad0
ec6710f
2224347
5ab4e52
07bef30
8d79a6b
24f4ca7
d939da5
b424064
a608415
105a53e
85039e8
6e14a53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,16 @@ import Type.Data.RowList (RLProxy(..)) | |
-- | - Associativity: `(x <> y) <> z = x <> (y <> z)` | ||
-- | | ||
-- | One example of a `Semigroup` is `String`, with `(<>)` defined as string | ||
-- | concatenation. | ||
-- | concatenation. Another example is `List a`, with `(<>)` defined as | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is definitely a good example to include here 👍 |
||
-- | list concatenation. | ||
-- | | ||
-- | ### Newtypes for Semigroup | ||
-- | | ||
-- | There are two other ways to implement an instance for this type class | ||
-- | regardless of which type is used. These instances can be used by | ||
-- | wrapping the values in one of the two newtypes below: | ||
-- | 1. `First` - Use the first argument every time: `append first _ = first`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These instances are a bit weird and somewhat rarely used, so I'm not sure they deserve a mention here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say they deserve a mention because it mirrors what is said in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just think that of all the things we might want to say about semigroups to someone encountering the concept for the first time, I think the First and Last semigroups should be fairly low down on the list, even if their newtypes do happen to live in this repository. While having some level of understanding of the Semigroup type class is more or less required if you want to use PureScript, I would argue that the First and Last semigroups are very much not required. I think it’s more respectful of the user’s time and energy to avoid mentioning things if we don’t expect that they will find them useful at the moment they are reading the docs in question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then again, they are simple examples, and giving plenty of simple examples is always an important part of teaching these concepts, so maybe they do deserve a mention here. |
||
-- | 2. `Last` - Use the last argument every time: `append _ last = last`. | ||
class Semigroup a where | ||
append :: a -> a -> a | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should save the specifics of each Monoid-related newtype to the definitions of the newtypes themselves. I suspect it be easier for people to understand these if they can see the rendered definitions alongside the documentation which says how they work, and also this would be very easy to get out of sync since the newtypes themselves live in separate files.
I do think this is a good place to mention the issue that some types permit multiple Monoid instances, and that we can express this with newtypes, but I think it would be best to keep that discussion minimal here. Something like "For example, the Additive newtype has a Monoid instance which is based on Semiring's
plus
andzero
, and the Multiplicative newtype has a Monoid instance which is based on Semiring'smul
andone
. There are a number of similar newtypes in the submodules of Data.Monoid in this package.Also, I realise this is a bit of a nitpick, but I would also prefer not to call it a "workaround", because I think doing this means admitting that not being able to equip a type with two Monoid instances is a deficiency (I don't think it is).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
Does my rendering work? Or are there other changes you would prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 👍