Release/1.1.1 #167
Release/1.1.1 #167
Conversation
| } | ||
| } | ||
|
|
||
| responseResult.attempt.map { |
There was a problem hiding this comment.
I quite like .handleError instead of .attempt for this kind of thing
| ): F[Either[RegistryError, A]] = { | ||
| val responseResult = client.run(req).use[F, Either[RegistryError, A]] { | ||
| case Status.Successful(response) => | ||
| response.as[A].map(_.asRight) |
There was a problem hiding this comment.
I think .as[A] can raise an error in F if the body cannot be parsed as expected. (I'm only 80% certain on that - need to check). Is it OK to let the error bubble up, or should this be caught immediately and converted to one of the failure classes?
There was a problem hiding this comment.
That's a good point. I think likely we are stumbling upon this infamous #152 because the body is trimmed.
| response | ||
| .as[A] | ||
| .map(_.asRight[RegistryError]) | ||
| .handleError { |
There was a problem hiding this comment.
I know this is exactly what I just said to do..... but I'm looking at it again and wondering if it was the right thing 🤦♂️. Because you already handle the error later on at line 134. And also it's inconsistent, because there are other places where you don't catch errors that might get raised (e.g. response.bodyText.compile.string). I guess you've added an informative error message, so maybe it is better.
Oh also..... I think that methods like handleError are guaranteed to be called with non-fatal exceptions only. So it shouldn't be necessary to pattern match on NonFatal. However, I never found good documentation on this - I think I just read it somewhere in a gitter channel.
I don't suggest changing anything though. This is all good.
There was a problem hiding this comment.
Yeah, I think there's still a slight difference that would help us to catch trimmed payloads.
On line 134 I catch very IO-ish client exceptions, i.e. network is unavailable, connection pool issue etc. Whereas this handleError is specifically about things like decoding failure due trimming or dropped connection and we'll get different error messages.
Although that's post-rationalisation - I also haven't realised the exceptions is caught.
No description provided.