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

Adding partitionEitherM #2641

Merged
merged 18 commits into from
Jan 22, 2019
Merged

Conversation

blast-hardcheese
Copy link
Contributor

@blast-hardcheese blast-hardcheese commented Nov 26, 2018

  • Used partitionEitherM in a project to ensure there weren't any odd usability concerns
  • Added Foldable[F].partitionEitherM[G, A, B, C]
  • Duplicated all partitionEither tests and modified them to test partitionEitherM
  • Ran sbt test
  • Ran sbt prePR

@blast-hardcheese blast-hardcheese changed the title Adding partitionEither Adding partitionEitherM Nov 26, 2018
@blast-hardcheese
Copy link
Contributor Author

blast-hardcheese commented Nov 26, 2018

Getting an unrelated error due to decimal precision in JS the JVM variants:

[info] Tests:
[info] - CommutativeGroup[BigDecimal].commutativeGroup.associative *** FAILED ***
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (Discipline.scala:14)
[info]     Falsified after 96 successful property evaluations.
[info]     Location: (Discipline.scala:14)
[info]     Occurred when passed generated values (
[info]       arg0 = 5.182509090085532E-184,
[info]       arg1 = -5.036270751383713E-218,
[info]       arg2 = 2.0611576410519167E-208
[info]     )
[info]     Label of failing property:
[info]       Expected: 5.182509090085532000000002061157641E-184
[info]       Received: 5.182509090085532000000002061157640E-184

@codecov-io
Copy link

codecov-io commented Nov 29, 2018

Codecov Report

Merging #2641 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2641      +/-   ##
==========================================
- Coverage   95.15%   95.14%   -0.02%     
==========================================
  Files         365      365              
  Lines        6774     6794      +20     
  Branches      297      314      +17     
==========================================
+ Hits         6446     6464      +18     
- Misses        328      330       +2
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/foldable.scala 100% <100%> (ø) ⬆️
...patTest/src/main/scala/catsBC/MimaExceptions.scala 0% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Kleisli.scala 97.29% <0%> (+0.02%) ⬆️
core/src/main/scala/cats/syntax/apply.scala 57.14% <0%> (+32.14%) ⬆️

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 b4d1ad1...43e4276. Read the comment docs.

@ceedubs
Copy link
Contributor

ceedubs commented Dec 3, 2018

Thanks, @blast-hardcheese! This looks good to me, but introduces a binary incompatibility, so we'll have to figure out how to handle that. We'll either need to temporarily add it on as a syntax extension or wait until Cats 2.0 to merge it. cc @kailuowang @LukaJCB

@LukaJCB
Copy link
Member

LukaJCB commented Dec 3, 2018

I think making this a syntax extension is the better path :) WDYT @blast-hardcheese?

@blast-hardcheese
Copy link
Contributor Author

@LukaJCB I wouldn't be opposed to that, but that would introduce the same binary incompatibility, just for the syntax class, wouldn't it?

@LukaJCB
Copy link
Member

LukaJCB commented Dec 4, 2018

Not quite, you can get it working by creating a new trait that gets mixed into the various imports :)
You can find a lot of examples if you search for e.g. BinCompat1 in the codebase

@blast-hardcheese
Copy link
Contributor Author

Resolved conflict with another BinCompat that showed up in the interim

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

You are extending the type class instance, while normally we would just add a syntax extension to the data type. Since another binary compatible change PR #2292 is just merged, you can take advantage of the workaround structure added there and simply change your partitionEitherM accordingly and add to FoldableOps0,

@blast-hardcheese
Copy link
Contributor Author

@kailuowang Understood. I was on the fence about how this method should be consumed, as I try to strike a good balance between convenience and discoverability for my teammates. Are there objections to having both?

@blast-hardcheese
Copy link
Contributor Author

For the time being, I've made the changes you suggested

@kailuowang
Copy link
Contributor

kailuowang commented Dec 29, 2018

@blast-hardcheese I see, I am not against having both. A sbt fmt would probably fix the build.

@blast-hardcheese blast-hardcheese force-pushed the partitionEitherM branch 2 times, most recently from a32ddbe to f997256 Compare December 29, 2018 06:50
@kailuowang kailuowang added this to the 1.6 milestone Jan 12, 2019
* res1: (List[Nothing], List[Int]) = (List(),List(4, 8, 12, 16))
* }}}
*/
def partitionEitherM[G[_], A, B, C](fa: F[A])(f: A => G[Either[B, C]])(implicit A: Alternative[F],
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is duplicated in two places. What if instead we added a partitionEitherM method to the Foldable companion object and had the FoldableOps0 method delegate to it? At that point, I'm not sure if it makes sense to have the FoldableOps1 extension to the type class itself; people could just call the method on the companion object if they wanted it. WDYT @kailuowang and @blast-hardcheese?

Sorry I'm sure that this is annoying. I think that Cats maintainers need to put some effort into our approach to binary compatibility and making it easier on other contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consolidating definitely felt like the right approach, but doing the same syntax lookup again while we're already working around bin compat seemed worse. I can make the change.

* res1: (List[Nothing], List[Int]) = (List(),List(4, 8, 12, 16))
* }}}
*/
def partitionEitherM[G[_], A, B, C](fa: F[A])(f: A => G[Either[B, C]])(implicit A: Alternative[F],
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to generalize this to any Bifoldable instead of just Either?

  def partitionBiffyM[G[_], H[_,_], A, B, C](fa: F[A])(f: A => G[H[B, C]])(implicit A: Alternative[F],
                                                                         M: Monad[G], H: Bifoldable[H]): G[(F[B], F[C])] = {
    import cats.instances.tuple._

    implicit val mb: Monoid[F[B]] = A.algebra[B]
    implicit val mc: Monoid[F[C]] = A.algebra[C]

    F.foldMapM[G, A, (F[B], F[C])](fa)(
      a =>
        M.map(f(a)){
          H.bifoldMap[B, C, (F[B], F[C])](_)(
            b => (A.pure(b), A.empty[C]),
            c => (A.empty[B], A.pure(c)))
      }
    )
  }

Then you could use it for the partitionEitherM use-case, but you could also use it for other Bifoldables like Tuple2:

scala> Foldable[List].partitionBiffyM(list)(a => Eval.now((a - 1, a + 1)))
res5: cats.Eval[(List[Int], List[Int])] = cats.Eval$$anon$6@67bef13e

scala> res5.value
res6: (List[Int], List[Int]) = (List(0, 1, 2, 3),List(2, 3, 4, 5))

I'm hoping that someone else has a better name than mine 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to suggest a broader refactor that includes partitionEither. I can generalize to partitionBiffy as well as partitionBiffyM, while providing the concretized partitionEitherM that just defers to partitionBiffyM, if that is desirable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it a shot, though the examples are getting increasingly convoluted I think.

@kailuowang
Copy link
Contributor

looks like this one can use another sbt fmt

@blast-hardcheese
Copy link
Contributor Author

@kailuowang just pushed a fix -- I seem to remember somewhere seeing that it's not desired to force push to branches in cats, is this a hard rule? I'd really like to squash some of the noise, only keeping the relevant commits

@kailuowang
Copy link
Contributor

kailuowang commented Jan 17, 2019

thanks @blast-hardcheese! generally speaking, it's more preferable to not change commit history of a PR. It's easier for reviewers if your new changes come in as new commits, otherwise they will have to review everything from scratch. We simply squash everything on merge, that way the history on master is clean.

@blast-hardcheese
Copy link
Contributor Author

Understood.

@LukaJCB
Copy link
Member

LukaJCB commented Jan 17, 2019

I just want to preface this, that I really appreciate the time you take to make this PR perfect, so thanks a ton @blast-hardcheese!
I'm really sorry to continue the bikeshedding here, especially after you've already made a ton of changes to your PR, but can we name the functions partitionBifold instead?
Sounds nicer to me than bifoldable and also reflects the implementation, we partition using some structure which supports a bifold, so it seems nicer and more accurate IMO.

Again thanks for your work so far! :)

@ceedubs
Copy link
Contributor

ceedubs commented Jan 18, 2019

Agreed. Thanks a lot, @blast-hardcheese!

LukaJCB
LukaJCB previously approved these changes Jan 18, 2019
@LukaJCB
Copy link
Member

LukaJCB commented Jan 18, 2019

Can you run scalafmt once more? :)

@blast-hardcheese
Copy link
Contributor Author

Done

Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Thanks a lot for this really high quality PR, @blast-hardcheese!

* scala> val list = List(1,2,3,4)
* scala> list.partitionBifold(a => (a, "value " + a.toString))
* res0: (List[Int], List[String]) = (List(1, 2, 3, 4),List(value 1, value 2, value 3, value 4))
* `Const`'s second parameter is never instantiated, so we can use an impossible type:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should line 231 have a // at the beginning of the comment? I guess that I don't know whether or not that's necessary in a doctest example; just wanted to make sure that this code is running when tests are run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just assumed the parser rules were similar to literate extensions for languages. Altering line 232 to have a syntax error caused coreJVM/test:compile to fail to compile, which was good as it seems as though there are other instances of this: https://github.com/typelevel/cats/blob/774fb51/core/src/main/scala/cats/Foldable.scala#L48

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should say, I'm not opposed to changing it, just that it currently isn't causing the tool to stop compiling the other lines.

}
}

test("Foldable#partitionEitherM consistent with List#partition") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh this is a good idea for a test 👍

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Awesome! Anyone have any objections to merging this as is? :)

@kailuowang kailuowang merged commit a332935 into typelevel:master Jan 22, 2019
@blast-hardcheese blast-hardcheese deleted the partitionEitherM branch January 22, 2019 17:55
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.

5 participants