Repository navigation
Use type classes instead of overloads in Local - #977
Conversation
| */ | ||
| def bind[R](ctx: Context)(f: => R): R = | ||
| macro Macros.localLet | ||
| def bind[R: CanIsolate](ctx: Context)(f: => R): R = { |
There was a problem hiding this comment.
I don't know why mima complains about this change but not for isolate
There was a problem hiding this comment.
It complains because this is an actual method, whereas isolate is a macro.
Changing isolate right now will not break binary compat.
This means that this bind needs to be deprecated. Can you try the hard source deprecation trick via private[Local]?
There was a problem hiding this comment.
Not sure what's the trick but I think I managed to get rid of binary incompatibility without exposing old methods
|
@alexandru @oleg-py I made a PoC with type class I didn't do it for
|
Note that sometimes usage of Given that // Should fail at compile time, unless we import something explicitly like:
import monix.execution.Local.Implicits.synchronous
Local.isolate {
local.get
}And maybe we can allow def printIt(): Unit =
someContext.execute(() => {
println(local.get)
})
local := 100
context.execute(() => {
Local.isolate {
local := 200
// Will this see 100 or 200?
printIt()
}
// Will this see 100 or 200?
printIt()
}) |
|
In other words, I'm thinking it might be a good idea to only have global implicits for |
…onix into Avasil-locals-protect-from-error
Note that it's also related to asynchronicity, IIRC the problem was caused by the timing of Other asynchronous code that can't be mapped should be fine as well: local := 100
context.execute(() => {
Local.isolate {
local := 200
// Will this see 100 or 200?
printIt()
}
// Will this see 100 or 200?
printIt()
})In this example, we set local to 100, then we create a new Local reference with value of the original local. it is set to 200 and printed. Print outside of isolated block acts on original reference so it prints 100, as expected. The problem would occur if a |
|
Thanks @alexandru I merged it and added basic tests for |
|
@alexandru after merging with your PR travis always fails with and I have no idea how to debug it :( have similar thing on cats-effect 2.0 branch |
|
Yes, one of the tests is screwed, I noticed it but haven't gotten around to fixing it. Will take a look tonight. |
| * Instance for `java.util.concurrent.CompletableFuture`. | ||
| */ | ||
| implicit def completableFuture[R]: CanBindLocals[CompletableFuture[R]] = | ||
| CompletableFutureInstance.asInstanceOf[CanBindLocals[CompletableFuture[R]]] |
There was a problem hiding this comment.
Can we do this on 2.11? It's not guaranteed to have Java 8, which is when CompletableFuture was added.
There was a problem hiding this comment.
Ah, good point. You're right, it's only available starting with Java 8.
|
I'm on board, but leave default "any A" instance as a non-implicit, so people can adjust their code if necessary without copy-pasting pieces of |
|
I agree, however I'm working on an Standby please 🙂 |
|
Hi guys, I've made changes on this PR: Can you please take a look? |
|
@alexandru I went through it quickly and looks good, I think we could go forward with it |
Remove Task/Coeval instances
|
@alexandru |
|
Oleg also approves so I merged PR to my fork and we could go forward. I will be travelling tomorrow but I should be available tomorrow in the early evening if we can make a release, I have already prepared release notes. |
It is released for 2.13.0. Should we release it for 2.13.0-M5? What's the holdup for us releasing Monix for 2.13 anyway? Cats, Cats-Effect? What's the status there? |
|
Yeah, cats supports 2.13 only for 2.0 which is currently in RC. I don't know what are the blockers |
|
So how do we proceed? I think the easiest way is to just release Alternatively we can drop |
|
We should drop |
|
What's the holdup on Cats-Effect 2.0 and Scala 2.13 anyway? |
Cats-Effect 2.0 supports Scala 2.13 in A holdup on |
|
@alexandru I disabled I can't reproduce it locally. Any idea? |
|
@Avasil I've cleared Travis cache for your branch, seems to be running now. |
|
Thank you @oleg-py ! @alexandru Can I do a release today? :) Waiting for Cats-Effect 2.0 could save us small headache in the future but it will take indefinite amount of time and we are on |
|
@Avasil I know you are impatient and so am I. The situation is ridiculous. However:
So I'm thinking that if you we want a release today, it should be another RC 🙂 People have been waiting since 18 Mar 2018 for the final version, surely they can wait another week or two. In the meantime I'll get involved and make sure that Cats 2.0 and Cats-Effect 2.0 are pushed forward. |
|
@alexandru |
|
For Cats-Effect 2.0 RC I would, however Cats 2.0 is actively developed. There's an outstanding issue with So no, not yet. If people want Cats-Effect 2.0-RC, they can just force it in their build. |
|
Cool, I'm merging this PR then I'm not able to release today but I can make it tomorrow early. Should we also announce feature freeze so nothing except bug fixes and Cats upgrade will be added in this period? |
|
Yes, feature freeze + |
|
@Avasil btw, I think we still have an issue with this PR that I forgot to fix: Will make another quick PR tonight. |
|
@alexandru It also seems like |
* Protect from error in Local.bind * Use type class instead of overloads and pattern matching * add mima filters * scalafmt * fix compilation error in Task * Fix binary incompatibilities * Local proposal * Add more tests * Move CompletableFuture tests to scala_2.12 suites * Remove Task/Coeval instances * Default implicit for CanBindLocals * Fix doc * Fix comment * Disable 2.13.0-M5
Proof of concept with:
CanIsolatetype class which describesbindandisolateComment from Alex's prs:
Localshould have side-effectful behavior, should not suspend execution, otherwise its usage is misleading.I've added an import for synchronous behavior:
What this does is to import in scope an implicit for all unknown
CanBindLocals[A]types, but that does NOT override the implicits that are already in scope. This is important because we don't want to override theFutureinstance via an import like this.Towards this purpose I've published a new micro library that introduces the
Notdata type, for providing evidence that an implicit does not exist for a certain type:https://github.com/monix/implicitbox
This is a very light dependency and introducing it in Monix felt wrong.