Follow up to #751 #757

milessabin · 2015-12-16T10:10:45Z

Added Reducible instance for OneAnd.
Added Alternative#compose and MonoidK#compose.

codecov-io · 2015-12-16T10:50:27Z

Current coverage is `87.77%`

Merging #757 into master will increase coverage by +0.16% as of ae7e7ac

@@            master    #757   diff @@
======================================
  Files          166     166       
  Stmts         2293    2299     +6
  Branches        74      74       
  Methods          0       0       
======================================
+ Hit           2009    2018     +9
  Partial          0       0       
+ Missed         284     281     -3

Review entire Coverage Diff as of ae7e7ac

Powered by Codecov. Updated on successful CI builds.

milessabin · 2015-12-16T11:42:34Z

I don't know how to interpret that additional Missed line ... can someone help?

ceedubs · 2015-12-16T12:22:32Z

core/src/main/scala/cats/MonoidK.scala

+  extends MonoidK[λ[α => F[G[α]]]] {
+
+  implicit def F: MonoidK[F]
+  implicit def G: MonoidK[G]


We don't actually use this G at all. The same is true for the current SemigroupK - if you drop the SemigroupK[G] implicit evidence, everything still compiles.

We can freely take a SemigroupK[F] and turn it into a SemigroupK[λ[α => F[G[α]]] with no requirements on G (it can even be a polykinded type like Nothing or Any!). I think we should probably drop the requirements on G. I'm also curious if another name would make more sense, since we aren't actually composing two MonoidK instances together. However, we are creating a MonoidK instance for the composite type, so it still may be a reasonable name.

Good point ... I'll remove the constraint.

There is usage of the from MonoidK[F] compose MonoidK[G] (resp. SemigroupK) which depends on the constraint generating an implicit argument which which be supplied explicitly. I'll try providing two variants, one with the constraint and one without.

I'm not sure I understand this comment. I'm saying that I think we should be able to change MonoidK[F] compose MonoidK[G] to simply MonoidK[F].compose[G] since the MonoidK[G] is ignored as far as I can tell. I might be missing something.

What I meant is that there is usage of the form f compose g for this entire family. I think we should preserve that, and also add an operator of the form f.composedWith[G] for the cases where the constraint isn't necessary.

Hmm I guess that seems to be a bit misleading to me. You aren't really composing the instances at all. You are just grabbing a type parameter from the second one. My inclination would be to have def compose[G[_]]: Semigroup[...] on SemigroupK, def compose[G[_]]: MonoidK[...] on MonoidK, def compose[G[_]:Applicative: Applicative[...] on Applicative, and def compose[G[_]:Applicative]: Alternative[...] on Alternative. To me this seems to be the clearest about what's going on and which constraints are needed.

@non I think you originally added the SemigroupK[G] requirement to compose. Was that intentional or an oversight? What are your thoughts?

Edit: for the cases where the evidence isn't required, I'm completely happy with calling these something other than compose to avoid ambiguity.

…operation.

milessabin · 2015-12-16T13:00:28Z

I've added the unconstrained variants as composedWith taking only a type argument and no value argument. I'm open to alternative suggestions for the name. I think it makes sense to stick with compose for the constrained versions.

ceedubs · 2015-12-16T13:17:06Z

core/src/main/scala/cats/Alternative.scala

+  /**
+   * Compose two Alternative intsances.
+   */
+  def compose[G[_]](implicit GG: Alternative[G]): Alternative[λ[α => F[G[α]]]] =


I think the GG constraint can be relaxed to Applicative while still returning an Alternative (also on CompositeAlternative below).

Agreed ... updated accordingly ...

milessabin · 2015-12-17T10:11:29Z

@ceedubs @non I'd like to move this forward if possible. I think the binary compose methods on SemigroupK and MonoidK are nice to have from a consistency PoV, but I'll take them out if they're blocking this.

We do need the renamed version with just an explicit type argument and no value argument though, and it should have a name other than compose. Do you have any suggestions? We're going from a MonoidK[F] to a MonoidK[λ[t => F[G[t]]]], so if it isn't composition, is it some sort of lifting or wrapping operation?

milessabin · 2015-12-29T10:44:27Z

Bump ...

non · 2016-01-04T05:21:00Z

👍 from me, generally speaking.

I would be fine using a name like curryType (or even curry) or any of your suggestions (e.g. wrap). It definitely seems like a useful operation.

milessabin · 2016-01-04T08:11:53Z

@non curryType? Is that comment for this PR or a different one?

ceedubs · 2016-01-05T13:54:16Z

Sorry for the delayed response. I've been on vacation.

I guess I still find am not convinced of the usefulness of the methods that take an instance but don't actually use it, and I'm concerned that they could result in unexpected behavior if someone is expecting a composition of effects from the two type constructors (and the ScalaDoc for MonoidK.compose currently says "Compose two MonoidK instances." which I think is misleading). If you have an example of why these methods are handy, please do share.

For the versions that take a type argument but no value, would composeK be a good name? It seems like these could be viewed as universal compositions in some sense.

mpilquist · 2016-01-30T15:42:39Z

@milessabin Now that #772 is merged, can you fix conflicts?

stew · 2016-01-30T16:18:36Z

I added PR milessabin#1 to patch up the merge

stew · 2016-01-30T16:35:27Z

👍

stew · 2016-01-30T17:05:55Z

actually cody has done a pretty good job of convincing me that although composeWith is useful, compose is not really.

ceedubs · 2016-01-30T17:19:13Z

Just to clarify a couple points:

I am not trying to convince anyone of anything. Just raising my concerns :)
I completely think compose should exist on the subtypes of Functor (such as Alternative), where the instance is actually used. It's the other places (such as MonoidK), where I don't see the value.

travisbrown · 2016-01-30T17:29:11Z

I agree that it makes sense not to include compose here (at least until someone can make a case for it), and I'm happy with either composeWith or composeK for the other name.

milessabin · 2016-01-31T08:31:44Z

I have some general concerns about the way we're using K as a suffix (TL;DR: the K implies something higher kind related which isn't quite right .. I think we're after a related, but distinct, notion) but at least composeK would be consistent with things that we do elsewhere, so I won't object.

ceedubs · 2016-01-31T15:59:51Z

It sounds like there is some consensus on this. Miles is currently on holiday, so I'll try to put together a PR that includes these changes along with the discussed updates.

Per feedback on typelevel#757.

Followup to #751 (replaces #757)

milessabin added 2 commits December 16, 2015 09:35

Added Reducible instance for OneAnd.

f6ded9e

Added Alternative#compose and MonoidK#compose.

5c67d9e

milessabin added the in progress label Dec 16, 2015

ceedubs reviewed Dec 16, 2015
View reviewed changes

Reinstate composedWith on MonoidK and SemigroupK as an unconstrained …

b415188

…operation.

Tweaked doc.

ec82947

ceedubs reviewed Dec 16, 2015
View reviewed changes

Weakened constraint of RHS of Alternative#compose to Applicative.

4aab63e

milessabin mentioned this pull request Dec 18, 2015

combine syntax appears to be behaving unexpectedly when kitchen sink importing #763

Closed

aryairani mentioned this pull request Dec 29, 2015

add Reducible instance for OneAnd, deprecate Foldable instance #772

Merged

stew mentioned this pull request Jan 30, 2016

Mergemiles milessabin/cats#1

Closed

travisbrown mentioned this pull request Jan 30, 2016

Let's talk the next release (0.4.0 edition) #834

Closed

5 tasks

ceedubs self-assigned this Jan 31, 2016

ceedubs added a commit to ceedubs/cats that referenced this pull request Jan 31, 2016

Update SemigroupK/MonoidK composition

58b3f5b

Per feedback on typelevel#757.

ceedubs mentioned this pull request Jan 31, 2016

Followup to #751 (replaces #757) #843

Merged

adelbertc closed this in #843 Feb 1, 2016

adelbertc added a commit that referenced this pull request Feb 1, 2016

Merge pull request #843 from ceedubs/751-followup

a369fa0

Followup to #751 (replaces #757)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up to #751 #757

Follow up to #751 #757

milessabin commented Dec 16, 2015

codecov-io commented Dec 16, 2015

milessabin commented Dec 16, 2015

ceedubs Dec 16, 2015

milessabin Dec 16, 2015

milessabin Dec 16, 2015

ceedubs Dec 16, 2015

milessabin Dec 16, 2015

ceedubs Dec 16, 2015

milessabin commented Dec 16, 2015

ceedubs Dec 16, 2015

milessabin Dec 16, 2015

milessabin commented Dec 17, 2015

milessabin commented Dec 29, 2015

non commented Jan 4, 2016

milessabin commented Jan 4, 2016

ceedubs commented Jan 5, 2016

mpilquist commented Jan 30, 2016

stew commented Jan 30, 2016

stew commented Jan 30, 2016

stew commented Jan 30, 2016

ceedubs commented Jan 30, 2016

travisbrown commented Jan 30, 2016

milessabin commented Jan 31, 2016

ceedubs commented Jan 31, 2016

Follow up to #751 #757

Follow up to #751 #757

Conversation

milessabin commented Dec 16, 2015

codecov-io commented Dec 16, 2015

Current coverage is 87.77%

milessabin commented Dec 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milessabin commented Dec 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milessabin commented Dec 17, 2015

milessabin commented Dec 29, 2015

non commented Jan 4, 2016

milessabin commented Jan 4, 2016

ceedubs commented Jan 5, 2016

mpilquist commented Jan 30, 2016

stew commented Jan 30, 2016

stew commented Jan 30, 2016

stew commented Jan 30, 2016

ceedubs commented Jan 30, 2016

travisbrown commented Jan 30, 2016

milessabin commented Jan 31, 2016

ceedubs commented Jan 31, 2016

Current coverage is `87.77%`