Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implements semigroup and monoid for Maybe. #125

Merged
merged 1 commit into from
May 30, 2017
Merged

Implements semigroup and monoid for Maybe. #125

merged 1 commit into from
May 30, 2017

Conversation

diasbruno
Copy link
Contributor

Implements semigroup and monoid for Maybe.

robotlolita
robotlolita reviewed May 26, 2017
View reviewed changes
src/data/maybe/maybe.js Outdated
@@ -42,6 +42,10 @@ const assertMaybe = assertType(Maybe);


extend(Just.prototype, {
empty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this definition.

src/data/maybe/maybe.js Outdated
@@ -52,6 +56,12 @@ extend(Just.prototype, {
}
});

extend(Nothing.prototype, {
empty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this one.

src/data/maybe/maybe.js Outdated
* type: |
* forall a: () => Maybe a
*/
empty: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this :)

src/data/maybe/maybe.js Outdated
* type: |
* forall a: () => Maybe a
*/
empty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're all automatically derived from this one definition here :)

@robotlolita
Copy link
Member

Looks neat, thanks! :)

I've requested a few simple changes and then it's ready to be merged :)

@diasbruno
Copy link
Contributor Author

hmm, I've forgot to remove those when you fixed the test issue. :)
Is anything else missing? (docs...)

@diasbruno diasbruno force-pushed the feature/maybe-semigroup-monoid branch from 377d18b to 16ce056 Compare May 27, 2017 01:18
@robotlolita
Copy link
Member

Nice 👍

Could you add an authors metadata to the methods you've added? Like:

/*~
 * authors:
 *   - "@diasbruno"
 * type: |
 *   ...
 */

If you're up to writing docs too, a short description with usage examples (as explained here) would be neat! In this case, the category for empty would be Constructing, and the category for concat would be Combining.

@diasbruno diasbruno force-pushed the feature/maybe-semigroup-monoid branch 3 times, most recently from 871a1d8 to 9261aaa Compare May 27, 2017 09:13
@diasbruno diasbruno force-pushed the feature/maybe-semigroup-monoid branch from 9261aaa to 7b51fcd Compare May 27, 2017 14:09
@diasbruno
Copy link
Contributor Author

I tried to write all the docs, but metamagical was not happy and it could not parse the full comment. So this should be ready to merge.

Hope to take some time to hack with metamagical (seems pretty cool).
I'll provide the error I got later.

@robotlolita
Copy link
Member

No worries :) Thanks

@robotlolita robotlolita merged commit 573c2bf into origamitower:master May 30, 2017
@ghost ghost removed the in progress label May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robotlolita robotlolita robotlolita approved these changes

Assignees
No one assigned
Labels
a:Base c:Core k:Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@diasbruno @robotlolita