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.

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

Monoidal improvements #740

Closed
adelbertc opened this issue Dec 11, 2015 · 4 comments
Closed

Monoidal improvements #740

adelbertc opened this issue Dec 11, 2015 · 4 comments

Comments

@adelbertc
Copy link
Contributor

The major pieces of adding Monoidal are in place, per https://github.com/non/cats/pull/555

[ ] As it stands, Monoidal is defined with just product (no pure) and as such only has the associativity law. We should perhaps have something similar to Apply/Applicative where we have a Semigroupal (?) with product and Monoidal extends Semigroupal with pure. If we go that route.. we'll have two different pures that should really be the same thing - one for Monoidal the other for Applicative.
[ ] Documentation

@OlivierBlanvillain
Copy link
Contributor

@julienrf I've just read through #555 changes and I have the following question, what's the reason to not implement the new product method introduced by Monoidal directly on Apply? I feel like all implementations of this method on types <: Apply could be factorized by the following, to be added directly on Apply:

def product[A, B](fa: F[A], fb: F[B]): F[(A, B)] =
  ap(fa)(map(fb)(b => (a => (a, b))))

@julienrf
Copy link
Contributor

julienrf commented Jan 9, 2016

Hi Olivier,

I just followed this comment (though I didn’t run any sort of benchmark).

@OlivierBlanvillain
Copy link
Contributor

@julienrf I'm really sorry for the stupid question, I didn't read through the discussion and only looked at the commit... I didn't run any benchmarks but was simply surprised by the addition of new abstract methods introduced on Apply, I now understand it's this way for performance reasons.

Did you consider the option of splitting MonoidalBuilder into 3? (I did something similar here, but the motivation was to improve error reporting because map/imap/contramap are all overloaded apply to be user facing). (I removed my table it did not make sense)

@julienrf
Copy link
Contributor

julienrf commented Jan 9, 2016

Yes, adding Divide and Divisible would be a good idea.

adelbertc added a commit to adelbertc/cats that referenced this issue Jan 12, 2016
adelbertc added a commit to adelbertc/cats that referenced this issue Jan 13, 2016
adelbertc added a commit that referenced this issue Jan 27, 2016
Add Semigroupal for Monoidal hierarchy, fixes #740
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

No branches or pull requests

3 participants