-
Notifications
You must be signed in to change notification settings - Fork 24
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
Download streams #41
Download streams #41
Conversation
Conflicts: README.md js/src/main/scala/fr/hmil/roshttp/BrowserDriver.scala js/src/main/scala/fr/hmil/roshttp/HttpDriver.scala js/src/main/scala/fr/hmil/roshttp/NodeDriver.scala jvm/src/main/scala/fr/hmil/roshttp/HttpDriver.scala shared/src/main/scala/fr/hmil/roshttp/AbstractDriver.scala shared/src/main/scala/fr/hmil/roshttp/HttpRequest.scala
getStringFromBuffer(buffer, charset) | ||
}) | ||
// TODO: We might not want to depend on monifu's Scheduler | ||
.asFuture |
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.
@sjrd Need your advice here. Monix needs an implicit Scheduler
in order to use asFuture. The Scheduler
is a subclass of ExecutionContext, which means that even though my interface allows the user to provide an ExecutionContext, I cannot use it because monix wants its own brand of ExecutionContext.
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.
Well, the enclosing apply
method takes an Observable
, so it depends on Monix anyway, right? In that case, I guess apply
should require an implicit Scheduler
rather than an ExecutionContext
.
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.
But what if the user uses his own ExecutionContext throughout the project and then imports RösHTTP. From our discussion in #34 , it seems that this is likely the case. In this situation the user is forced to use a Scheduler instead...
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.
Well, you can't have your cake and eat it too.
If you're basing yourself on top of Monix, you have to play by Monix' rules. If it wants an implicit Scheduler
, then yes, you need to demand a Scheduler
from your users. Unless you can internally synthesize a valid Scheduler
from an ExecutionContext
, but I doubt that (otherwise Monix would take ECs to begin with).
If you don't want to impose the Scheduler tax on your users (which, I agree, is desirable), then you cannot use parts of the Monix API that require Schedulers.
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.
Where's the definition of asFuture
? Maybe it can be generalized upstream to work with a normal EC.
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.
It seems that there is no way to avoid the Scheduler. Plus the user has to provide a Scheduler anyway if he wants to use the streaming interface.
I should then probably replace all ExecutionContext
parameters with Scheduler
instead to make the whole API more consistent.
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.
I should then probably replace all ExecutionContext parameters with Scheduler instead to make the whole API more consistent.
I don't think so. I think it's good to require as little as possible from your users. That way, say they're only using the batch APIs, they can use any EC they already have. If they're using the streaming API as well, they can have an implicit Scheduler
, and it will also be valid for methods requiring an implicit EC because Scheduler <: EC
.
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.
The thing is that both the batch and streaming APIs are backed by the same stream-based backend call (which is what this piece of code is about). This avoids code duplication, reduces the risks of bugs and does probably have little effect on performance for basic usages. Therefore, a Scheduler is required even for the batch API.
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.
Ah, well ... I guess then it doesn't really matter all that much.
Conflicts: README.md
Implementation of the long awaited download streams. Addresses half of #7 .
WIP status: