add attemptNarrow to ApplicativeErrorOps #2863

marcodippy · 2019-05-30T16:43:09Z

Hello, I already copy-pasted this function in a few places in my codebases, so I thought it could be useful to have it in cats.

Example use case (assuming F : ApplicativeError[F, Throwable]):

def authenticate(r: Request[F]): F[Unit] = ???

val authentication: Kleisli[F, Request[F], Either[AuthenticationException, Unit]] =
  Kleisli { request => authenticate(request).attemptCase[AuthenticationException] }

codecov-io · 2019-05-30T17:17:22Z

Codecov Report

Merging #2863 into master will decrease coverage by 1%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
- Coverage   94.18%   93.17%   -1.01%     
==========================================
  Files         368      372       +4     
  Lines        6948     7182     +234     
  Branches      308      198     -110     
==========================================
+ Hits         6544     6692     +148     
- Misses        404      490      +86

Flag	Coverage Δ
#scala_version_212	`93.52% <100%> (?)`
#scala_version_213	`90.85% <100%> (?)`

Impacted Files	Coverage Δ
core/src/main/scala/cats/ApplicativeError.scala	`100% <100%> (ø)`	⬆️
.../src/main/scala/cats/syntax/applicativeError.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/parallel.scala	`0% <0%> (-100%)`	⬇️
core/src/main/scala/cats/syntax/parallel.scala	`62.5% <0%> (-17.5%)`	⬇️
kernel/src/main/scala/cats/kernel/Monoid.scala	`72.72% <0%> (-16.17%)`	⬇️
...ernel/src/main/scala/cats/kernel/Semilattice.scala	`85.71% <0%> (-14.29%)`	⬇️
core/src/main/scala/cats/instances/sortedMap.scala	`84.74% <0%> (-12.76%)`	⬇️
core/src/main/scala/cats/data/NonEmptyChain.scala	`85.39% <0%> (-11.93%)`	⬇️
core/src/main/scala/cats/instances/sortedSet.scala	`88.23% <0%> (-11.77%)`	⬇️
core/src/main/scala/cats/data/Op.scala	`77.77% <0%> (-9.73%)`	⬇️
... and 103 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 0209330...f6e9228. Read the comment docs.

diesalbla · 2019-06-01T15:45:14Z

core/src/main/scala/cats/syntax/applicativeError.scala

@@ -83,6 +86,9 @@ final class ApplicativeErrorOps[F[_], E, A](private val fa: F[A]) extends AnyVal
  def attempt(implicit F: ApplicativeError[F, E]): F[Either[E, A]] =
    F.attempt(fa)

+  def attemptCase[EE](implicit F: ApplicativeError[F, E], tag: ClassTag[EE], ev: EE <:< E): F[Either[EE, A]] =
+    F.recover(F.map(fa)(_.asRight[EE])) { case e: EE => e.asLeft[A] }


Given that extra inter-dependencies could lead to circular dependencies, perhaps we can by-pass the either syntax and use this not much longer alternative:

Suggested change

F.recover(F.map(fa)(_.asRight[EE])) { case e: EE => e.asLeft[A] }

F.recover(F.map(fa)(x => Right[EE, A](x))) { case e: EE => Left[EE, A](e) }

kailuowang · 2019-06-21T14:53:27Z

I think this could be useful although I am not quite sure about the name. I am thinking attemptNarrow or narrowAttempt?

marcodippy · 2019-06-21T16:07:17Z

@kailuowang renamed to attemptNarrow 👍

kailuowang

Thanks!

tkroman · 2019-07-25T00:43:17Z

core/src/main/scala/cats/syntax/applicativeError.scala

@@ -83,6 +85,9 @@ final class ApplicativeErrorOps[F[_], E, A](private val fa: F[A]) extends AnyVal
  def attempt(implicit F: ApplicativeError[F, E]): F[Either[E, A]] =
    F.attempt(fa)

+  def attemptNarrow[EE](implicit F: ApplicativeError[F, E], tag: ClassTag[EE], ev: EE <:< E): F[Either[EE, A]] =


why do we need a classtag here?

EE would be erased and we wouldn't be able to pattern match on it otherwise @tkroman

tpolecat

I would like to see a test with a parameterized type, also demonstrating non-compilation for things like narrowing to List[Int] when it's actually List[String]. Since we're skating close to erasure.

marcodippy · 2019-08-02T21:58:30Z

@tpolecat I added the test, let me know what you think

marcodippy · 2019-10-22T18:51:51Z

hey @kailuowang, is there anything I can do to get this merged?

one more thing

kailuowang · 2019-10-23T01:07:53Z

Hi @marcodippy sorry about the delay here, but can I ask one more thing? Now that we dropped 2.11 from master branch, we can add this method to the typeclass itself and have the syntax delegate to it. Would you make that change?

marcodippy · 2019-10-23T07:56:25Z

no problem @kailuowang, I just pushed the change :)

kailuowang

Thanks!

The original implementation of the code to expose the `ConditionalCheckFailedException` was introduced with PR #212 in May 2018, but I'm not sure why the multi-line implementation was necessary - there's a discussion in PR #212, but I don't quite follow it: #212 (comment) The Cats `recover` functionality I'm using here was added with `ApplicativeError` in typelevel/cats#812 in January 2016, so it would have been available for PR #212. Perhaps it was some consequence of us being on AWS SDK v1 at that point, and having to use `AsyncHandler`? This implementation is definitely much more similar to ones we've used elsewhere, ie `ScalaFutureAdapter` and `PekkoInterpreter`. If we had a `ClassTag` we could go even smaller and use `attemptNarrow`, which was added to Cats with typelevel/cats#2863 in May 2019 - but even without that, the implementation is much smaller.

diesalbla reviewed Jun 1, 2019

View reviewed changes

add attemptCase to ApplicativeErrorOps

a75d0f9

marcodippy force-pushed the add_attemptcase branch from b6ba437 to a75d0f9 Compare June 2, 2019 08:10

add attemptNarrow to ApplicativeErrorOps

c1363bb

kailuowang changed the title ~~add attemptCase to ApplicativeErrorOps~~ add attemptNarrow to ApplicativeErrorOps Jun 21, 2019

kailuowang previously approved these changes Jun 21, 2019

View reviewed changes

tkroman reviewed Jul 25, 2019

View reviewed changes

kailuowang requested a review from LukaJCB August 2, 2019 16:36

LukaJCB previously approved these changes Aug 2, 2019

View reviewed changes

tpolecat suggested changes Aug 2, 2019

View reviewed changes

add test for attemptNarrow and parametrized types

3767ca3

marcodippy dismissed stale reviews from LukaJCB and kailuowang via 3767ca3 August 2, 2019 21:57

kailuowang previously approved these changes Aug 3, 2019

View reviewed changes

kailuowang requested a review from tpolecat August 15, 2019 20:39

LukaJCB previously approved these changes Oct 22, 2019

View reviewed changes

move attemptNarrow to ApplicativeError typeclass

f6e9228

marcodippy dismissed LukaJCB’s stale review via f6e9228 October 23, 2019 07:54

kailuowang approved these changes Oct 23, 2019

View reviewed changes

kailuowang merged commit f414c1d into typelevel:master Oct 23, 2019

travisbrown added the enhancement label Nov 5, 2019

travisbrown added this to the 2.1.0-RC1 milestone Nov 5, 2019

travisbrown mentioned this pull request Nov 13, 2019

Backport 2.1.0 changes for Scala 2.11 #3143

Closed

22 tasks

gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Feb 9, 2020

backported typelevel#2863 Add attemptNarrow to ApplicativeErrorOps

d98a401

travisbrown pushed a commit that referenced this pull request Feb 19, 2020

backported #2863 Add attemptNarrow to ApplicativeErrorOps (#3289)

5d97f98

travisbrown mentioned this pull request Mar 18, 2020

Consider removing attemptNarrow #3360

Open

rtyley mentioned this pull request Jul 23, 2024

Simplify CatsAdapter exception-handling scanamo/scanamo#1792

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add attemptNarrow to ApplicativeErrorOps #2863

add attemptNarrow to ApplicativeErrorOps #2863

marcodippy commented May 30, 2019

codecov-io commented May 30, 2019 •

edited

Loading

diesalbla Jun 1, 2019

kailuowang commented Jun 21, 2019

marcodippy commented Jun 21, 2019

kailuowang left a comment

tkroman Jul 25, 2019

marcodippy Jul 25, 2019

tpolecat left a comment

marcodippy commented Aug 2, 2019

marcodippy commented Oct 22, 2019

kailuowang commented Oct 23, 2019

marcodippy commented Oct 23, 2019

kailuowang left a comment

	F.recover(F.map(fa)(_.asRight[EE])) { case e: EE => e.asLeft[A] }
	F.recover(F.map(fa)(x => Right[EE, A](x))) { case e: EE => Left[EE, A](e) }

add attemptNarrow to ApplicativeErrorOps #2863

add attemptNarrow to ApplicativeErrorOps #2863

Conversation

marcodippy commented May 30, 2019

codecov-io commented May 30, 2019 • edited Loading

Codecov Report

diesalbla Jun 1, 2019

Choose a reason for hiding this comment

kailuowang commented Jun 21, 2019

marcodippy commented Jun 21, 2019

kailuowang left a comment

Choose a reason for hiding this comment

tkroman Jul 25, 2019

Choose a reason for hiding this comment

marcodippy Jul 25, 2019

Choose a reason for hiding this comment

tpolecat left a comment

Choose a reason for hiding this comment

marcodippy commented Aug 2, 2019

marcodippy commented Oct 22, 2019

kailuowang commented Oct 23, 2019

marcodippy commented Oct 23, 2019

kailuowang left a comment

Choose a reason for hiding this comment

codecov-io commented May 30, 2019 •

edited

Loading