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

Apply syntax for tuples #1487

Closed

Conversation

DavidGregory084
Copy link
Member

@DavidGregory084 DavidGregory084 commented Dec 8, 2016

Move apply syntax for tuples under cats.syntax.apply._ (previously cats.syntax.tuple._). Methods are now suffixed by N rather than the tuple's arity:

(fa, fb).mapN { case (a, b) =>  }
(fa, fb, fc).contramapN { case (a, b, c) =>  } 

@kailuowang
Copy link
Contributor

blocked by #1073

@peterneyens
Copy link
Collaborator

Now that we have enabled the partial unification fix (#1583) and dropped Unapply (#1679), I think we can move forward on this..

I would really like to see this in 1.0.0-MF. I think the sooner we add this, the better.

I think it might be nice to keep the cartesian builder (|@|) for at least until the next version (currently that probably will be 1.0.0-RC1), but deprecate it. Opinions?

Do you still have time to work on this @DavidGregory084? Otherwise I am happy to pick this up.

@peterneyens peterneyens added this to the 1.0.0-MF milestone May 19, 2017
@kailuowang
Copy link
Contributor

@peterneyens I am leaning slightly towards removing the cartesian builder (|@|) in 1.0.0-MF. Mainly because that 1.0.0-MF is a "proposal" for the final API in 1.0.0, it would be a bit weird to have a deprecated thing in the proposal.
In the past, we have been taking the "let's break things sooner rather than later" route. Maybe we do the same this time?

@peterneyens
Copy link
Collaborator

Makes sense. Just though that having a deprecation cycle before 1.0.0 might make things easier, especially for something like |@| which gets a lot of use.

Paging @johnynek who raised some upgrade/adoption concerns earlier in #1635 (comment).

@DavidGregory084 DavidGregory084 changed the title WIP Tuple Apply syntax fixes Apply syntax for tuples May 20, 2017
-XX:+CMSClassUnloadingEnabled
# must be enabled for CMSClassUnloadingEnabled to work
-XX:+UseConcMarkSweepGC
Copy link
Member Author

@DavidGregory084 DavidGregory084 May 20, 2017

Choose a reason for hiding this comment

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

Comments in here cause Could not find or load main class: # when starting sbt on Windows

@codecov-io
Copy link

codecov-io commented May 20, 2017

Codecov Report

Merging #1487 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1487   +/-   ##
=======================================
  Coverage   93.94%   93.94%           
=======================================
  Files         241      241           
  Lines        4096     4096           
  Branches      156      159    +3     
=======================================
  Hits         3848     3848           
  Misses        248      248
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/apply.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/data/EitherT.scala 96.52% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc4080c...eefd70b. Read the comment docs.

@DavidGregory084
Copy link
Member Author

@peterneyens @kailuowang Sure, I have updated the PR. I restored Cartesian builders, let's see how the discussion about deprecating them goes once everyone who's interested has weighed in.

@kailuowang kailuowang mentioned this pull request May 25, 2017
26 tasks
Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

👍 from me.

@kailuowang
Copy link
Contributor

kailuowang commented May 28, 2017

If we keep CartesianBuilder, should we mark them as deprecated and update the docs to use the new syntax?

@kailuowang
Copy link
Contributor

@DavidGregory084 I am just curious if you have time to continue on this, I think we reached a consensus that we just need to mark CartesianBuilder stuff as deprecated and update the docs to use new tupple syntax. Sorry for dragging so long and now trying to push it with release date past due. I'll gladly complete the rest if you want.

@DavidGregory084
Copy link
Member Author

@kailuowang Ah sorry, I wasn't clear that we had reached a consensus. :) I should have some time to work on this, but probably not until the end of the week, so if you think you are going to have time earlier than that please feel free to take this on.

@kailuowang
Copy link
Contributor

@DavidGregory084 thanks so much! I was just checking if I can find something to work on. Will leave this one to you then.

@kailuowang
Copy link
Contributor

@DavidGregory084 still have the bandwidth to finish up this one? I'll be more than happy to help if you don't.

@DavidGregory084
Copy link
Member Author

Thanks @kailuowang, I'm struggling a bit to find the time so it would be great if you could 👍

@kailuowang
Copy link
Contributor

no problem @DavidGregory084 , #1745 is submitted to finish up this one. Closing this for now.

@johnynek
Copy link
Contributor

I missed where we reached consensus to drop CartesianBuilder. Can we link to that here?

I think this is a bit unfortunate because I know of a few code bases using @ and this seems like a minor stylistic change that will put work on a lot of folks.

Removing Xor kept us on an old version of cats for a while at Stripe because the activation energy to go remove it everywhere was pretty high. I'm afraid this could be similar (though smaller in scope).

@kailuowang
Copy link
Contributor

@johnynek do you mind continue the discussion at #1745? I'll reply to you there.

kailuowang added a commit that referenced this pull request Jul 2, 2017
* Apply syntax for tuples, fixes #1363

* fix feedback and build error
@gabro
Copy link
Contributor

gabro commented Aug 10, 2017

I think this is a bit unfortunate because I know of a few code bases using @ and this seems like a minor stylistic change that will put work on a lot of folks.

Hey @johnynek I was going through the PRs that introduced breaking changes in 1.0.0 and I stumbled upon your comment. Did you have a chance to try the Scalafix rewrites? They should solve (or at least mitigate greatly) that specific problem :)

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

Successfully merging this pull request may close these issues.

7 participants