-
Notifications
You must be signed in to change notification settings - Fork 304
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
add generic type to FetchCatsBackend stub method #1889
add generic type to FetchCatsBackend stub method #1889
Conversation
@@ -48,5 +48,5 @@ object FetchCatsBackend { | |||
* | |||
* See [[SttpBackendStub]] for details on how to configure stub responses. | |||
*/ | |||
def stub[F[_]: Concurrent: ContextShift]: SttpBackendStub[F, Any] = SttpBackendStub(new CatsMonadAsyncError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR - following suit for other stub backends, I think it would be sufficient to simply type this using the WebSockets
capability. Then you can upcast to Any
because of variance. See e.g.:
sttp/effects/fs2/src/main/scalajvm/sttp/client3/httpclient/fs2/HttpClientFs2Backend.scala
Line 173 in d0fb7e9
def stub[F[_]: Async]: SttpBackendStub[F, Fs2Streams[F] with WebSockets] = SttpBackendStub(implicitly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no, thank you for the awesome lib, I'm just trying to give back a little 😄
For the reply, I'm not sure I get it. The docs for the second type parameter in SttpBackend
say
P – Capabilities supported by this backend, in addition to sttp.capabilities.Effect. This might be Any (no special capabilities), sttp.capabilities.Streams (the ability to send and receive streaming bodies) or sttp.capabilities.WebSockets (the ability to handle websocket requests).
So should the change be, this? (then we only support Websockets
? Not sure if FetchCatsBackend
can support anything else?)
def stub[F[_]: Async, WebSockets]: SttpBackendStub[F, WebSockets] = SttpBackendStub(new CatsMonadAsyncError)
or there should ne now change at all 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be changed as you described. In general this should be the maximal set of capabilities - you can always upcast a backend to narrow the supported capabilities list. In case of cats, it's only WebSockets, no streaming is supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay understood, thank you.
Code changed 😄
Thanks! :) |
Before submitting pull request:
sbt compile
sbt compileDocs
sbt test
sbt scalafmt
This PR adds a generic type to stub method in FetchCatsBackend companion object.
Reasoning for this change is, when you have
SttpBackend
defined as a dependency like thisit is not possible to mock it using this code
adding a type resolves this issue 😃