Skip to content

Scala 3 (Dotty) support - #1323

Merged
alexandru merged 20 commits into
series/3.xfrom
wip/dotty
May 10, 2021
Merged

alexandru merged 20 commits into
series/3.xfrom
wip/dotty

Conversation

@larsrh

@larsrh larsrh commented Nov 20, 2020

Copy link
Copy Markdown
Contributor

@larsrh larsrh mentioned this pull request Nov 20, 2020
7 tasks
@larsrh larsrh changed the title update build for Dotty Dotty support Nov 20, 2020
@larsrh
larsrh force-pushed the wip/dotty branch 5 times, most recently from eb5e75f to 0cef70d Compare November 23, 2020 20:43
@larsrh

larsrh commented Dec 6, 2020

Copy link
Copy Markdown
Contributor Author

There a still four compile errors in the doctest reactiveJVM:

[error] -- Error: /home/lars/proj/monix/monix/monix-reactive/jvm/target/scala-3.0.0-M1/src_managed/test/monix/reactive/ObservableDoctest.scala:446:11 
[error] 446 |    ).merge
[error]     |           ^
[error]     |no implicit argument of type monix.reactive.OverflowStrategy[B] was found for parameter os of method merge in class Observable

I find that strange; I thought the type ascription to the implicit argument of merge fixed these.

@Avasil

Avasil commented Dec 6, 2020

Copy link
Copy Markdown
Collaborator

There a still four compile errors in the doctest reactiveJVM:

[error] -- Error: /home/lars/proj/monix/monix/monix-reactive/jvm/target/scala-3.0.0-M1/src_managed/test/monix/reactive/ObservableDoctest.scala:446:11 
[error] 446 |    ).merge
[error]     |           ^
[error]     |no implicit argument of type monix.reactive.OverflowStrategy[B] was found for parameter os of method merge in class Observable

I find that strange; I thought the type ascription to the implicit argument of merge fixed these.

I've opened an issue about it scala/scala3#10497

@larsrh

larsrh commented Dec 6, 2020

Copy link
Copy Markdown
Contributor Author

I've opened an issue about it

Ah right, I forgot that existed 😁

@Avasil

Avasil commented May 4, 2021

Copy link
Copy Markdown
Collaborator

@larsrh I have finally spent some time on it and the good news is that almost all tests pass! I think the only issue left is Observable#merge...

@alexandru
alexandru marked this pull request as ready for review May 7, 2021 07:53
@alexandru alexandru changed the title Dotty support Scala 3 (Dotty) support May 7, 2021
@alexandru
alexandru marked this pull request as draft May 7, 2021 10:09
@alexandru
alexandru marked this pull request as ready for review May 7, 2021 11:40
@alexandru

Copy link
Copy Markdown
Member

I solved merge by fixing the tests 🤷‍♂️ as there's nothing we can do for now, the only other options being:

  1. to wait for that bug to get fixed in Scala 3, or ...
  2. break compatibility

We can break compatibility in 4.0.0, but for now this will have to do.


I also upgraded sbt, and cleaned up all warnings, except for those weird ones that aren't reported as actual warnings and that may be the result of some plugin.

Not sure if I want to do anything else before merging.

@larsrh

larsrh commented May 7, 2021

Copy link
Copy Markdown
Contributor Author

Please hold back with merging until I get the chance to do one final review pass, okay?

@alexandru

Copy link
Copy Markdown
Member

@larsrh sounds good, thanks!

Comment thread monix-eval/jvm/src/test/scala/monix/eval/TaskLocalJVMSuite.scala

@oleg-py oleg-py left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@larsrh larsrh left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noted a few issues where there are inconsistencies between Scala 2 and 3 and (possibly?) unrelated changes that I don't understand. But those aren't blockers. Good work, everyone 💪

Comment thread monix-execution/js/src/main/scala_3.0/monix/execution/atomic/AtomicNumber.scala Outdated
Comment thread monix-execution/js/src/main/scala_3.0/monix/execution/atomic/AtomicNumber.scala Outdated
Comment thread monix-execution/jvm/src/main/scala_3.0/monix/execution/atomic/Atomic.scala Outdated
Comment thread monix-execution/jvm/src/main/scala_3.0/monix/execution/atomic/Atomic.scala Outdated
Comment thread monix-execution/jvm/src/main/scala_3.0/monix/execution/atomic/Atomic.scala Outdated
Comment thread monix-execution/shared/src/main/scala_3.0/monix/execution/misc/Local.scala Outdated
Comment thread monix-java/src/main/scala/monix/java8/eval/package.scala
Comment thread monix-java/src/main/scala/monix/java8/execution/package.scala
Comment thread monix-java/src/main/scala/monix/java8/execution/package.scala
@alexandru
alexandru merged commit 651e7da into series/3.x May 10, 2021
@alexandru
alexandru deleted the wip/dotty branch May 10, 2021 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants