Skip to content
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

Update to scala 2.13 and akka 2.6 (incremental) #1390

Merged
merged 4 commits into from
Apr 27, 2020

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Apr 22, 2020

This builds on #1389, so the changeset is minimal (disregard the first commit).

Main changes:

  • relaxed compiler parameters to minimize impact (e.g. allow
    deprecated features)
  • scala.collection.JavaConverters -> scala.jdk.CollectionConverters
  • MultiMap -> MultiDict

Compilation is 25% faster on my machine, compiler is a bit more strict
(it found an "invalid comparison" bug).

pom.xml Outdated Show resolved Hide resolved
@@ -132,16 +132,12 @@
<version>3.4.2</version>
<configuration>
<args combine.children="append">
<arg>-deprecation</arg>
<!--arg>-Xlint:deprecation</arg-->
Copy link
Member

Choose a reason for hiding this comment

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

Maybe create a tracking issue to revert that back in the future (once all warnings are fixed)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason why I'm not fixing the warning is that it would break source compatibility with scala 2.11.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but we probably want to get rid of it "at some point" (once the android branch is gone), just want to make sure we don't forget it (we're not looking at the pom very often to be honest)

Copy link
Member Author

Choose a reason for hiding this comment

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

we're not looking at the pom very often to be honest

Alright. I wouldn't worry about it though, my OCD is already in overdrive with those:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Done with #1393.

@@ -174,6 +174,7 @@ package object eclair {
override def toFloat(x: MilliSatoshi): Float = x.toLong
override def toDouble(x: MilliSatoshi): Double = x.toLong
override def compare(x: MilliSatoshi, y: MilliSatoshi): Int = x.compare(y)
override def parseString(str: String): Option[MilliSatoshi] = ???
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's a problem to leave this unimplemented.

Comment on lines +118 to +122
<dependency>
<groupId>org.scala-lang.modules</groupId>
<artifactId>scala-collection-contrib_${scala.version.short}</artifactId>
<version>0.2.1</version>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is required for MultiDict because MultiMap has been removed.

@pm47 pm47 force-pushed the scala2.13-akka2.6-incremental branch from 45d35b7 to 3213dda Compare April 24, 2020 13:54
@pm47
Copy link
Member Author

pm47 commented Apr 24, 2020

Rebased.

@pm47 pm47 requested a review from t-bast April 24, 2020 13:54
.travis.yml Outdated
@@ -4,7 +4,7 @@ services:
dist: trusty
language: scala
scala:
- 2.11.12
- 2.13.1
Copy link
Member

Choose a reason for hiding this comment

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

Why not even 2.13.2? #reckless?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch. Main pom.xml was also using 2.13.1. Fixed in ed5f7c7.

eclair-node/pom.xml Show resolved Hide resolved
t-bast
t-bast previously approved these changes Apr 27, 2020
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM

This is almost a drop-in replacement. I had to relaxed compiler
parameters to allow deprecated features though.

Main changes:
- relaxed compiler parameters to minimize impact (e.g. allow
deprecated features)
- `scala.collection.JavaConverters` -> `scala.jdk.CollectionConverters`
- `MultiMap` -> `MultiDict`

Compilation is 25% faster on my machine, compiler is a bit more strict
(it found an "invalid comparison" bug).
@pm47
Copy link
Member Author

pm47 commented Apr 27, 2020

Rebased.

@pm47 pm47 requested a review from t-bast April 27, 2020 10:56
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

🚀

@pm47 pm47 merged commit 19975d3 into master Apr 27, 2020
@pm47 pm47 deleted the scala2.13-akka2.6-incremental branch April 27, 2020 11:12
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.

None yet

3 participants