-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Syntax for Cartesian operations with tuples #1112
Conversation
Current coverage is 90.58% (diff: 100%)@@ master #1112 diff @@
==========================================
Files 243 243
Lines 3293 3292 -1
Methods 3235 3236 +1
Messages 0 0
Branches 56 53 -3
==========================================
Hits 2982 2982
+ Misses 311 310 -1
Partials 0 0
|
The point if I'm not mistaken is to be able to write My thoughts are:
I guess another option is have a |
Thanks @DavidGregory084! Some perhaps relevant (but rushed because I have to go) notes:
|
afe548a
to
4f681ae
Compare
Thanks @yilinwei, @ceedubs. I tidied up the naming in this PR to follow @kailuowang's latest changes to disambiguate syntax implicits. I also made a small change to the boilerplate generation to allow multiple blocks to be expanded separately, e.g. trait TupleCartesianSyntax {
implicit def catsSyntaxTuple1Cartesian[F[_], A0](t1: Tuple1[F[A0]]): Tuple1CartesianOps[F, A0] = new Tuple1CartesianOps(t1)
implicit def catsSyntaxTuple2Cartesian[F[_], A0, A1](t2: Tuple2[F[A0], F[A1]]): Tuple2CartesianOps[F, A0, A1] = new Tuple2CartesianOps(t2)
// etc.
}
private[syntax] final class Tuple1CartesianOps[F[_], A0](t1: Tuple1[F[A0]]) {
def map[Z](f: (A0) => Z)(implicit functor: Functor[F]): F[Z] = functor.map(t1._1)(f)
def contramap[Z](f: Z => (A0))(implicit contravariant: Contravariant[F]): F[Z] = contravariant.contramap(t1._1)(f)
def imap[Z](f: (A0) => Z)(g: Z => (A0))(implicit invariant: Invariant[F]): F[Z] = invariant.imap(t1._1)(f)(g)
def apWith[Z](f: F[(A0) => Z])(implicit apply: Apply[F]): F[Z] = apply.ap(f)(t1._1)
}
private[syntax] final class Tuple2CartesianOps[F[_], A0, A1](t2: Tuple2[F[A0], F[A1]]) {
def map2[Z](f: (A0, A1) => Z)(implicit functor: Functor[F], cartesian: Cartesian[F]): F[Z] = Cartesian.map2(t2._1, t2._2)(f)
def contramap2[Z](f: Z => (A0, A1))(implicit contravariant: Contravariant[F], cartesian: Cartesian[F]): F[Z] = Cartesian.contramap2(t2._1, t2._2)(f)
def imap2[Z](f: (A0, A1) => Z)(g: Z => (A0, A1))(implicit invariant: Invariant[F], cartesian: Cartesian[F]): F[Z] = Cartesian.imap2(t2._1, t2._2)(f)(g)
def apWith[Z](f: F[(A0, A1) => Z])(implicit apply: Apply[F]): F[Z] = apply.ap2(f)(t2._1, t2._2)
}
// etc. It seems like there are conflicting concerns here about name clashes vs. consistency. |
@DavidGregory084, @ceedubs I think that, that would depend on #1073.Do you mind checking that My own thoughts on #1073 is that we should assume that people include in in there project as I'm not aware of any downsides to this. If it works with SI-2712 and it's decided that we assume that pre 2.12 projects use the plugin then I would be in favour of removing the I don't think anyone is in love with extra syntax though I could be wrong 😄. |
@yilinwei yeah, that's a good idea, I'll see if I can do that within the next couple of days |
@ceedubs Wouldn't I like the approach taken in this PR. And I agree that if it works reliably I'd like to deprecate the 👍 once @DavidGregory084 is happy with the PR. |
@non in scalaz it has both, which I find a bit convenient but it's fine if we don't do that in cats. The |
I like this approach. I have some thoughts around the name |
We could also call the implicit methods here |
I'm fine with either |
Yeah, I agree, I think we should distinguish from |
I am fine with either |
I'd vote |
Thanks for all the review, guys 👍 @yilinwei @non as expected this syntax works fine for e.g. Either with the SI-2712 plugin, but without it requires a type alias - it seems that deprecating With respect to the naming, |
Hmm that's a pretty good point @DavidGregory084 :). Personally I kind of like |
@ceedubs I quite like I have to admit that personally I am kind of attracted to |
@DavidGregory084 I'm perfectly happy with either |
I'm OK with |
@DavidGregory084 Are you still able to work on this? On a side note, due to the issues in choose |
@adelbertc yes, very sorry for sitting on this one, I've been dealing with some family illness so this fell by the wayside; should be able to take a look this evening. We would still need |
@DavidGregory084 I'm sorry to hear that, I hope everything is OK. No worries at all, family comes first. If you'd like, just say the word and I or another contributor can handle the rest of this patchset. Good point on |
4f681ae
to
df80438
Compare
@adelbertc Yes, a bit of normality has been restored now, hence why I've been looking at my GH notifications. You guys have been busy! 😄 0.7.0 looks awesome. As you are the only person who has expressed a strong preference about the naming so far (and I have a slight preference for this too) I have updated this PR and stuck with |
@@ -40,6 +40,7 @@ trait AllSyntax | |||
with TransLiftSyntax | |||
with TraverseFilterSyntax | |||
with TraverseSyntax | |||
with TupleSyntax |
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 needs to also have object tuple extends TupleSyntax
in package.scala
right?
Also, do we want to put this under tuples or Cartesian syntax?
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.
Ah yes, well spotted - thanks! I'll update this.
I am not really sure about the second question; my reasoning was that in general the syntax is named according to the type or set of types that it decorates but I may have got the wrong idea and I'm happy to rename it.
df80438
to
1181a33
Compare
1181a33
to
b6bd29a
Compare
LGTM 👍 I'm not sure what others thoughts are, but I feel like this should be under |
👍 |
Seems OK to me. 👍 I agree with @adelbertc that we may want to move it but I'm OK merging this then making a decision. |
Generates
Things I'm not sure about:
map2
,map3
, etc. or justmap
?