Skip to content

Conversation

@evilsoft
Copy link
Contributor

@evilsoft evilsoft commented Aug 29, 2017

A little Group therapy for the masses

image

This PR addresses this issue by adding a specification for Group. In addition to the new spec, I did the following to get the tests to work using "mocks" already in place:

  • Moved the internal/string_sum to internal/sum and modified it to work with Number instead of String.
  • Had to modify the monoid and traverable.composition specs to accommodate the newly Number dependentSum.
  • Fixed up the semigroupoid spec to run. The spec was not included in the specs, it is now.

### Group

A value that implements the Group specification must also implement
the [Monoid](#monoid) specification.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's more than one way to get to group, but we definitely shouldn't let that get in the way of this PR

(We could eventually have

class Magma m where
  concat :: m -> m -> m

class Magma m => Quasigroup m where
  latin :: (m, m) -> (m, m)

class Magma m => Unital m where
  empty :: () -> m

class Magma m => Semigroup m

class (Semigroup m, Unital m) => Monoid m
class (Quasigroup m, Unital m) => Loop m
class (Semigroup, Quasigroup, Unital m) => Group m

All (non-empty) associative quasigroups are groups, so there's no 8th member)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh snap. I see some possible opportunities here for another couple PRs!
Good call!

@evilsoft
Copy link
Contributor Author

@gabejohnson 🏫 me on how to get dat png updated.
Will get that in there today and take the WIP of of this.

@evilsoft
Copy link
Contributor Author

evilsoft commented Aug 30, 2017

Also, I am not satisfied with the names rightInverse and leftInverse any suggestions for better/established names for those?

EDIT:
okay, never mind, I found it! will be changing these to right cancellation and left cancellation

ANOTHER EDIT:
Cancel (hehe) that, cancellation properties are not the droids I am looking for, looks like I am still open for names for these properties. Any suggestions?

@evilsoft
Copy link
Contributor Author

Okay. Removed WIP, this is GTG and 🔴 👁 for review.

Functor -> Traversable;
Plus -> Alternative;
Semigroup -> Monoid;
Semigroup -> Monoid -> Group;
Copy link
Member

Choose a reason for hiding this comment

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

Please move this to a separate line, alphabetically please.

Functor -> Traversable;
Monoid -> Group;
Plus -> Alternative;
Semigroup -> Monoid;

@evilsoft
Copy link
Contributor Author

@joneshf addressed

Functor -> Extend;
Functor -> Profunctor;
Functor -> Traversable;
Monoid -> Group;
Copy link
Member

Choose a reason for hiding this comment

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

👍 Thanks!

@evilsoft
Copy link
Contributor Author

Any other feedback or anything I should address that is keeping this from being merged in?

@xaviervia
Copy link

What's the status of this one? It looks good :D

@joneshf
Copy link
Member

joneshf commented Oct 13, 2017

Agreed. We can follow up if something is missing/wrong.

@joneshf joneshf merged commit 9d5eaa8 into fantasyland:master Oct 13, 2017
@gabejohnson
Copy link
Member

A little late to the party. I think there's a small mistake

  1. g.concat(g.invert()) is equivalent to g.empty() (right inverse)
  2. g.invert().concat(g) is equivalent to g.empty() (left inverse)

I think it should be either g.constructor.empty() or G.empty().

See https://github.com/fantasyland/fantasy-land#plus

@davidchambers davidchambers mentioned this pull request Oct 13, 2017
@davidchambers
Copy link
Member

Good catch, @gabejohnson. See #275.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants