-
Notifications
You must be signed in to change notification settings - Fork 205
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
Unblock bloop - upgrade monix and bsp4s #1759
Conversation
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 started reviewing a bit, but I still have a way to go. Great work!
@@ -305,8 +301,10 @@ final class BloopBspServices( | |||
compileProvider = Some(BloopBspServices.DefaultCompileProvider), | |||
testProvider = Some(BloopBspServices.DefaultTestProvider), | |||
runProvider = Some(BloopBspServices.DefaultRunProvider), | |||
debugProvider = None, // todo kpodsiad |
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.
Turn it into a issue?
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.
@dos65 what about this one?
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.
heh - this could be easily fixed - created #1781
@tgodzik 😸 I wanted to test it a bit locally with Metals and provide more clean explanation/motivation before asking for the review but yeah - go ahead |
Ok, tested a bit it with metals - looks like it works fine. |
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 went through the entire PR, that is a lot of great work! I left some questions however to clarify some points and let me understand the PR better.
frontend/src/main/scala/bloop/util/jsoniter/JsoniterCodecs.scala
Outdated
Show resolved
Hide resolved
150d6d6
to
fa215fc
Compare
Replace `monix.eval.Task` usage on `bloop.task.Task`. This allows to keep the same cancellation behaviour as it was in monix2. Co-authored-by: Kamil Podsiadlo <kpodsiadlo@virtuslab.com>
Co-authored-by: Kamil Podsiadlo <kpodsiadlo@virtuslab.com>
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.
LGTM! Let's try it out in Metals!
@@ -305,8 +301,10 @@ final class BloopBspServices( | |||
compileProvider = Some(BloopBspServices.DefaultCompileProvider), | |||
testProvider = Some(BloopBspServices.DefaultTestProvider), | |||
runProvider = Some(BloopBspServices.DefaultRunProvider), | |||
debugProvider = None, // todo kpodsiad |
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.
@dos65 what about this one?
Actually, I forgot about this comment: Could you update it with an issue in a follow up @dos65 ? |
@tgodzik oh, yep - will do |
Follow up to scalacenter#1759 I thought that it was fixed in scalacenter@2baf28a but it seems that I got confused in locally published bloop versions.
No description provided.