Add PartialFunction instance for Profunctor typeclass #3392

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

resolves #3386

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

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

+     */
+    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?

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

core/src/main/scala/cats/arrow/Arrow.scala

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

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? :)

core/src/main/scala/cats/arrow/Arrow.scala

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

…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

core/src/main/scala/cats/arrow/Compose.scala

@@ -41,9 +41,12 @@ import scala.annotation.implicitNotFound
 }

 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

core/src/main/scala-2.12/cats/instances/all.scala

@@ -10,6 +10,7 @@ abstract class AllInstancesBinCompat
    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

core/src/main/scala/cats/Applicative.scala Show resolved Hide resolved

gagandeepkalra commented Apr 16, 2020

View reviewed changes

core/src/main/scala/cats/arrow/Arrow.scala Outdated Show resolved Hide resolved

gagandeepkalra commented Apr 16, 2020

View reviewed changes

core/src/main/scala/cats/instances/partialFunction.scala Show resolved Hide resolved

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

core/src/main/scala/cats/arrow/Arrow.scala Outdated Show resolved Hide resolved

core/src/main/scala/cats/instances/partialFunction.scala Outdated Show resolved Hide resolved

core/src/main/scala/cats/instances/partialFunction.scala Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PartialFunction instance for Profunctor typeclass #3392

Add PartialFunction instance for Profunctor typeclass #3392

gagandeepkalra commented Apr 13, 2020 •

edited

Loading

gagandeepkalra Apr 13, 2020

codecov-io commented Apr 13, 2020

LukaJCB left a comment

LukaJCB commented Apr 13, 2020

gagandeepkalra commented Apr 16, 2020

LukaJCB left a comment

codecov-commenter commented Jun 3, 2020 •

edited

Loading

gagandeepkalra commented Jun 3, 2020

barambani commented Jun 3, 2020

LukaJCB left a comment

barambani commented Jun 5, 2020

LukaJCB commented Jun 5, 2020

travisbrown left a comment

travisbrown Jun 11, 2020

gagandeepkalra Jun 11, 2020

travisbrown Jun 11, 2020 •

edited

Loading

gagandeepkalra Jun 11, 2020 •

edited

Loading

travisbrown Jun 12, 2020

travisbrown commented Jun 12, 2020

travisbrown commented Jun 12, 2020

travisbrown left a comment

gagandeepkalra commented Jun 12, 2020

travisbrown left a comment

travisbrown commented Jun 16, 2020

Add PartialFunction instance for Profunctor typeclass #3392

Add PartialFunction instance for Profunctor typeclass #3392

Conversation

gagandeepkalra commented Apr 13, 2020 • edited Loading

gagandeepkalra Apr 13, 2020

Choose a reason for hiding this comment

codecov-io commented Apr 13, 2020

Codecov Report

LukaJCB left a comment

Choose a reason for hiding this comment

LukaJCB commented Apr 13, 2020

gagandeepkalra commented Apr 16, 2020

LukaJCB left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 3, 2020 • edited Loading

Codecov Report

gagandeepkalra commented Jun 3, 2020

barambani commented Jun 3, 2020

LukaJCB left a comment

Choose a reason for hiding this comment

barambani commented Jun 5, 2020

LukaJCB commented Jun 5, 2020

travisbrown left a comment

Choose a reason for hiding this comment

travisbrown Jun 11, 2020

Choose a reason for hiding this comment

gagandeepkalra Jun 11, 2020

Choose a reason for hiding this comment

travisbrown Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

gagandeepkalra Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

travisbrown Jun 12, 2020

Choose a reason for hiding this comment

travisbrown commented Jun 12, 2020

travisbrown commented Jun 12, 2020

travisbrown left a comment

Choose a reason for hiding this comment

gagandeepkalra commented Jun 12, 2020

travisbrown left a comment

Choose a reason for hiding this comment

travisbrown commented Jun 16, 2020

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