Add PartialFunction instance for Profunctor typeclass by gagandeepkalra · Pull Request #3392 · typelevel/cats

gagandeepkalra · 2020-04-13T02:40:51Z

resolves #3386

gagandeepkalra · 2020-04-13T02:42:02Z

+     */
+    override def dimap[A, B, C, D](fab: PartialFunction[A, B])(f: C => A)(g: B => D): PartialFunction[C, D] =
+      new PartialFunction[C, D] {
+        override def isDefinedAt(c: C): Boolean = fab.isDefinedAt(f(c))


I don't think if there's any way out of running f twice

codecov-io · 2020-04-13T03:01:53Z

Codecov Report

Merging #3392 into master will decrease coverage by 0.22%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #3392      +/-   ##
==========================================
- Coverage   92.70%   92.48%   -0.23%     
==========================================
  Files         379      379              
  Lines        7981     7960      -21     
  Branches      230      226       -4     
==========================================
- Hits         7399     7362      -37     
- Misses        582      598      +16

Flag	Coverage Δ
#scala_version_212	`?`
#scala_version_213	`92.48% <75.00%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
...rc/main/scala/cats/instances/partialFunction.scala	`75.00% <75.00%> (ø)`
core/src/main/scala/cats/data/OneAnd.scala	`79.10% <0.00%> (-16.49%)`	⬇️
core/src/main/scala/cats/Reducible.scala	`97.22% <0.00%> (-2.78%)`	⬇️
core/src/main/scala/cats/syntax/either.scala	`81.61% <0.00%> (-1.72%)`	⬇️
...rc/main/scala/cats/laws/discipline/arbitrary.scala	`98.29% <0.00%> (-0.86%)`	⬇️
core/src/main/scala/cats/Show.scala	`67.56% <0.00%> (-0.86%)`	⬇️
core/src/main/scala/cats/instances/try.scala	`86.44% <0.00%> (-0.45%)`	⬇️
core/src/main/scala/cats/data/Tuple2K.scala	`91.30% <0.00%> (-0.19%)`	⬇️
.../src/main/scala/cats/laws/discipline/MiniInt.scala	`97.22% <0.00%> (-0.08%)`	⬇️
core/src/main/scala/cats/data/NonEmptyChain.scala	`92.92% <0.00%> (-0.08%)`	⬇️
... and 11 more

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 def9a6f...d2edf3f. Read the comment docs.

LukaJCB

Thanks for this! :)

LukaJCB · 2020-04-13T21:20:46Z

I wonder if we could add instances for the other parts of the arrow hierarchy. Seems like we could implement pretty much everything, no?

gagandeepkalra · 2020-04-16T10:06:54Z

@LukaJCB I updated the same PR instead, is it okay?

LukaJCB

Hey @gagandeepkalra sorry for keeping you waiting for so long, this looks amazing, thanks! Do you have time to rebase with master? :)

…y (available to all)

codecov-commenter · 2020-06-03T15:58:37Z

Codecov Report

Merging #3392 into master will increase coverage by 0.42%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master    #3392      +/-   ##
==========================================
+ Coverage   91.63%   92.05%   +0.42%     
==========================================
  Files         382      383       +1     
  Lines        8304     8347      +43     
  Branches      232      210      -22     
==========================================
+ Hits         7609     7684      +75     
+ Misses        695      663      -32

gagandeepkalra · 2020-06-03T16:00:03Z

Thank you @LukaJCB @barambani for the review, learnt something new today.

barambani · 2020-06-03T16:32:18Z

Thank you.

learnt something new today.

same here.

LukaJCB

Hey, thanks! This looks great, but I wonder why you only placed it in the companion objects of Profunctor and Compose, maybe I'm missing something?

barambani · 2020-06-05T14:18:12Z

@LukaJCB I suggested it to align to the approach taken here #3043

LukaJCB · 2020-06-05T14:52:48Z

Ah of course! Sorry I misremembered a detail of the whole thing

travisbrown

👍 to the concept—I'm not sure why we hadn't done this before. I do have a couple of comments / questions about details.

travisbrown · 2020-06-11T10:56:38Z


 object Compose {
-  implicit def catsInstancesForFunction1: ArrowChoice[Function1] with CommutativeArrow[Function1] =
+  implicit val catsInstancesForFunction1: ArrowChoice[Function1] with CommutativeArrow[Function1] =


It's not really a big deal, but there is a reason for these to be methods instead of vals: they're already instantiated as vals in the instances objects, so using def here won't result in new instantiations, but using a val does result in a little extra overhead and indirection, since it generates both an accessor method and a field.

In general this is unlikely to matter much, but because there are a lot of these, we've had a policy of def instead of val.

I see, that's a nice way of saving up on a reference; apologies for oversight

travisbrown · 2020-06-11T10:57:52Z

    with AllInstancesBinCompat4
    with AllInstancesBinCompat5
    with AllInstancesBinCompat6
+    with AllInstancesBinCompat7


We shouldn't need any new BinCompat traits now that we've dropped 2.11. You should be able to add PartialFunctionInstances directly to AllInstances, I believe?

I'm not sure, getting this error when adding to AllInstances; I don't see anything obvious, might need some help

[error] Cats core: Failed binary compatibility check against org.typelevel:cats-core_2.12:1.0.1! Found 2 potential problems (filtered 481)
[error] * abstract synthetic method cats$instances$PartialFunctionInstances$setter$catsStdInstancesForPartialFunction_=(cats.arrow.ArrowChoice)Unit in interface cats.instances.PartialFunctionInstances is inherited by class AllInstances in current version.
[error] filter with: ProblemFilters.excludeInheritedNewAbstractMethodProblem
[error] * abstract method catsStdInstancesForPartialFunction()cats.arrow.ArrowChoice in interface cats.instances.PartialFunctionInstances is inherited by class AllInstances in current version.
[error] filter with: ProblemFilters.excludeInheritedNewAbstractMethodProblem

@gagandeepkalra Hmm, thanks, I'll take a look now.

travisbrown · 2020-06-12T08:34:57Z

Oh, right. The issue is that adding a val to a trait still breaks bincompat even on 2.12+, because of synthetic methods. We've had to work around this in a couple of other places—let me find a link…

travisbrown · 2020-06-12T08:40:13Z

Here's some explanation of the issue: #2789 (comment)

I think that something like this should work:

trait PartialFunctionInstances {
  def catsStdInstancesForPartialFunction: ArrowChoice[PartialFunction] with CommutativeArrow[PartialFunction] =
    PartialFunctionInstances.instance
}

private object PartialFunctionInstances {
  private val instance: ArrowChoice[PartialFunction] with CommutativeArrow[PartialFunction] =
    new ArrowChoice[PartialFunction] with CommutativeArrow[PartialFunction] {
      // ...

I'd be happy to merge this PR as-is, and I can fix this particular part in a follow-up, if you'd prefer?

travisbrown

There's one remaining issue, but I think it could be handled in a follow-up PR, so I'm approving this now. Thanks again @gagandeepkalra!

…improved doctests with less imports now needed

gagandeepkalra · 2020-06-12T12:01:01Z

@travisbrown thank you, summarising-

I understand why adding a def to trait won't break bc given we have method defaults in java 8 interface, while though val would still require each class this trait is mixed in with, recompilation.

Meanwhile I had another doc-test import related improvement, so have pushed both together.

travisbrown

@gagandeepkalra Thanks, that looks good to me! I don't it necessarily needs a second re-review, but I'll leave it for right now for @LukaJCB to take a look if he wants.

I don't remember the details about why adding a val to a trait breaks bincompat in 2.12, but right, it has something to default methods (and the implementation of initialization using static forwarder methods on the companion object).

travisbrown · 2020-06-16T07:46:44Z

Going ahead and merging this since it had an earlier second 👍.

gagandeepkalra commented Apr 13, 2020

View reviewed changes

LukaJCB previously approved these changes Apr 13, 2020

View reviewed changes

gagandeepkalra dismissed LukaJCB’s stale review via ff84eea April 16, 2020 10:01

gagandeepkalra commented Apr 16, 2020

View reviewed changes

Comment thread core/src/main/scala/cats/Applicative.scala

gagandeepkalra commented Apr 16, 2020

View reviewed changes

Comment thread core/src/main/scala/cats/arrow/Arrow.scala Outdated

gagandeepkalra commented Apr 16, 2020

View reviewed changes

Comment thread core/src/main/scala/cats/instances/partialFunction.scala

gagandeepkalra requested a review from LukaJCB April 16, 2020 10:06

LukaJCB previously approved these changes Jun 2, 2020

View reviewed changes

barambani reviewed Jun 2, 2020

View reviewed changes

Comment thread core/src/main/scala/cats/arrow/Arrow.scala Outdated

Comment thread core/src/main/scala/cats/instances/partialFunction.scala Outdated

Comment thread core/src/main/scala/cats/instances/partialFunction.scala

gagandeepkalra added 4 commits June 3, 2020 21:47

added PartialFunction instance for Profunctor typeclass

05a861d

updated doctest to use as extension method

c6e9f47

added entire arrow heirarchy for PartialFunction

cfb8ead

addressed review feedback, moved implicits to the top of the hierarch…

6af528d

…y (available to all)

gagandeepkalra dismissed LukaJCB’s stale review via 6af528d June 3, 2020 15:57

gagandeepkalra force-pushed the profunctor/addPartialFunctionInstance branch from ff84eea to 6af528d Compare June 3, 2020 15:57

added back instance check when lifting a function to partialFunction

982c652

gagandeepkalra requested a review from LukaJCB June 5, 2020 11:12

LukaJCB reviewed Jun 5, 2020

View reviewed changes

LukaJCB previously approved these changes Jun 5, 2020

View reviewed changes

travisbrown suggested changes Jun 11, 2020

View reviewed changes

addressed review feedback, have val as def instead

e815d83

gagandeepkalra dismissed LukaJCB’s stale review via e815d83 June 11, 2020 16:29

travisbrown self-requested a review June 12, 2020 08:40

travisbrown previously approved these changes Jun 12, 2020

View reviewed changes

updated to have PartialFunctionInstances mixed in with AllInstances; …

4fd8927

…improved doctests with less imports now needed

gagandeepkalra dismissed travisbrown’s stale review via 4fd8927 June 12, 2020 11:59

gagandeepkalra requested review from LukaJCB and travisbrown June 12, 2020 12:02

travisbrown approved these changes Jun 12, 2020

View reviewed changes

travisbrown mentioned this pull request Jun 15, 2020

Add Parallel traverseFilter functions #3467

Merged

travisbrown merged commit 97468fa into typelevel:master Jun 16, 2020

travisbrown added the enhancement label Jun 17, 2020

travisbrown added this to the 2.2.0-M3 milestone Jun 17, 2020

gagandeepkalra deleted the profunctor/addPartialFunctionInstance branch June 18, 2020 01:30

Uh oh!

Conversation

gagandeepkalra commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gagandeepkalra Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 13, 2020

Codecov Report

Uh oh!

LukaJCB left a comment

Choose a reason for hiding this comment

Uh oh!

LukaJCB commented Apr 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gagandeepkalra commented Apr 16, 2020

Uh oh!

LukaJCB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gagandeepkalra commented Jun 3, 2020

Uh oh!

barambani commented Jun 3, 2020

Uh oh!

LukaJCB left a comment

Choose a reason for hiding this comment

Uh oh!

barambani commented Jun 5, 2020

Uh oh!

LukaJCB commented Jun 5, 2020

Uh oh!

travisbrown left a comment

Choose a reason for hiding this comment

Uh oh!

travisbrown Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

gagandeepkalra Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

travisbrown Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagandeepkalra Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbrown Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

travisbrown commented Jun 12, 2020

Uh oh!

travisbrown commented Jun 12, 2020

Uh oh!

travisbrown left a comment

Choose a reason for hiding this comment

Uh oh!

gagandeepkalra commented Jun 12, 2020

Uh oh!

travisbrown left a comment

Choose a reason for hiding this comment

Uh oh!

travisbrown commented Jun 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

gagandeepkalra commented Apr 13, 2020 •

edited

Loading

codecov-commenter commented Jun 3, 2020 •

edited

Loading

travisbrown Jun 11, 2020 •

edited

Loading

gagandeepkalra Jun 11, 2020 •

edited

Loading