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.

Sign up for GitHub

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

Jump to bottom

Scala 3.3.1 and improve Scala 3 compatibility #3010

Merged
merged 16 commits into from
Sep 18, 2023
Merged

Scala 3.3.1 and improve Scala 3 compatibility #3010

merged 16 commits into from
Sep 18, 2023

Conversation

Arikuti
Copy link
Contributor

@Arikuti Arikuti commented Sep 4, 2023

Update Deps

References #xxxx

johanandren
johanandren reviewed Sep 5, 2023
View reviewed changes
...cala/akka/stream/alpakka/googlecloud/bigquery/scaladsl/spray/BigQueryCollectionFormats.scala Outdated
import scala.reflect.ClassTag

trait BigQueryCollectionFormats {

/**
/**Ø
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**Ø
/**

google-common/src/main/scala/akka/stream/alpakka/google/http/GoogleHttp.scala Outdated
private implicit def system = http.system
private implicit def ec = system.dispatcher
private implicit def scheduler = system.scheduler
private implicit def system: ExtendedActorSystem = http.system
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this needs to be extended actorsystem, just ActorSystem is probably enough.

google-common/src/main/scala/akka/stream/alpakka/google/http/GoogleHttp.scala Outdated
private implicit def ec = system.dispatcher
private implicit def scheduler = system.scheduler
private implicit def system: ExtendedActorSystem = http.system
private implicit def ec: ExecutionContextExecutor = system.dispatcher
Copy link
Member

Choose a reason for hiding this comment

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

We usually type these as ExecutionContext as that is what is needed on the consuming side.

Copy link
Member

Choose a reason for hiding this comment

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

(same for all other dispatcher as implicit ec in the PR)

mqtt-streaming/src/test/scala/docs/scaladsl/MqttActorSystemsSpec.scala Outdated
@@ -31,7 +34,7 @@ class MqttTypedActorSystemSpec extends AnyWordSpec {

class MqttClassicActorSystemSpec extends AnyWordSpec {

implicit val actorSystem = akka.actor.ActorSystem("MqttClassicActorSystemSpec")
implicit val actorSystem: actor.ActorSystem = akka.actor.ActorSystem("MqttClassicActorSystemSpec")
Copy link
Member

Choose a reason for hiding this comment

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

Just use the full path and skip the package import

project/Dependencies.scala
Copy link
Member

Choose a reason for hiding this comment

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

Please revert the library (and Scala) bumps that are not strictly needed for this PR

ennru
ennru reviewed Sep 6, 2023
View reviewed changes
Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

All the other code changes look good.

If we bump Scala versions which doesn't seem strictly required for this, we need to update the link-validator config.

Did you intend to enable Scala 3 compilation on the modules you fixed?

...scala/akka/stream/alpakka/googlecloud/bigquery/scaladsl/spray/BigQueryRestBasicFormats.scala Outdated
Comment on lines 17 to 39
implicit val IntJsonFormat: json.DefaultJsonProtocol.IntJsonFormat.type = DefaultJsonProtocol.IntJsonFormat
implicit val FloatJsonFormat: _root_.spray.json.DefaultJsonProtocol.FloatJsonFormat.type =
DefaultJsonProtocol.FloatJsonFormat
implicit val DoubleJsonFormat: _root_.spray.json.DefaultJsonProtocol.DoubleJsonFormat.type =
DefaultJsonProtocol.DoubleJsonFormat
implicit val ByteJsonFormat: _root_.spray.json.DefaultJsonProtocol.ByteJsonFormat.type =
DefaultJsonProtocol.ByteJsonFormat
implicit val ShortJsonFormat: _root_.spray.json.DefaultJsonProtocol.ShortJsonFormat.type =
DefaultJsonProtocol.ShortJsonFormat
implicit val BigDecimalJsonFormat: _root_.spray.json.DefaultJsonProtocol.BigDecimalJsonFormat.type =
DefaultJsonProtocol.BigDecimalJsonFormat
implicit val BigIntJsonFormat: _root_.spray.json.DefaultJsonProtocol.BigIntJsonFormat.type =
DefaultJsonProtocol.BigIntJsonFormat
implicit val UnitJsonFormat: _root_.spray.json.DefaultJsonProtocol.UnitJsonFormat.type =
DefaultJsonProtocol.UnitJsonFormat
implicit val BooleanJsonFormat: _root_.spray.json.DefaultJsonProtocol.BooleanJsonFormat.type =
DefaultJsonProtocol.BooleanJsonFormat
implicit val CharJsonFormat: _root_.spray.json.DefaultJsonProtocol.CharJsonFormat.type =
DefaultJsonProtocol.CharJsonFormat
implicit val StringJsonFormat: _root_.spray.json.DefaultJsonProtocol.StringJsonFormat.type =
DefaultJsonProtocol.StringJsonFormat
implicit val SymbolJsonFormat: _root_.spray.json.DefaultJsonProtocol.SymbolJsonFormat.type =
DefaultJsonProtocol.SymbolJsonFormat
Copy link
Member

Choose a reason for hiding this comment

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

This could be better with the old import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes planning to enable Scala 3 compilation for modules in next PR.

Copy link
Member

Choose a reason for hiding this comment

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

It would make more sense to enable Scala 3 for these modules in this PR as it is the driving factor for this work.

patriknw
patriknw reviewed Sep 7, 2023
View reviewed changes
project/Dependencies.scala Outdated
val Scala3 = "3.2.2"
val Scala213 = "2.13.11" // update even in link-validator.conf
val Scala212 = "2.12.18"
val Scala3 = "3.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

The Scala 3 version is one of those that we should align across all Akka modules. We can make this change but should be aware of that it means that downstream, such as the Cassandra plugin, must also be updated before using this version of Alpakka. Created an issue akka/akka#32093

@johanandren johanandren mentioned this pull request Sep 7, 2023
Closed
@ennru ennru force-pushed the update-scala-version-and-deps branch from 2f0e944 to 15c24e6 Compare September 18, 2023 11:18
@ennru
Copy link
Member

ennru commented Sep 18, 2023

I rebased this PR as we merged Scala 3 support for AWS modules recently.

@ennru
Copy link
Member

ennru commented Sep 18, 2023

Scalaunidoc is unhappy with the backticks.

[error] java.net.URISyntaxException: Illegal character in path at index 107: [https://github.com/akka/alpakka/tree/main/google-common/src/main/scala/akka/stream/alpakka/google/scaladsl/`X-Upload-Content-Type`.scala#L17](https://github.com/akka/alpakka/tree/main/google-common/src/main/scala/akka/stream/alpakka/google/scaladsl/%60X-Upload-Content-Type%60.scala#L17)
[error] 	at java.base/java.net.URI$Parser.fail(URI.java:2913)

See https://github.com/akka/alpakka/actions/runs/6221787998/job/16884423957?pr=3010#step:6:416

But this did not change?!

@ennru
Copy link
Member

ennru commented Sep 18, 2023

Now this conflates the purpose of this PR with updates due to the more exact checks in Scala 2.13.12. I guess that should be separate. I'll go back to Scala 2.13.10 for this PR.

@ennru ennru force-pushed the update-scala-version-and-deps branch from c599944 to a8b3391 Compare September 18, 2023 16:10
@ennru
Copy link
Member

ennru commented Sep 18, 2023

Bumping to Scala 2.13.12 will require good Scala 3 compatibility, we can merge this PR as another step towards it.

ennru
ennru approved these changes Sep 18, 2023
View reviewed changes
Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru changed the title Update Scala Version Scala 3.3.1 and improve Scala 3 compatibility Sep 18, 2023
@ennru ennru merged commit ca9af1b into akka:main Sep 18, 2023
50 checks passed
@ennru
Copy link
Member

ennru commented Sep 18, 2023

Thank you @Arikuti for getting this started. Let's continue to move more modules towards Scala 3 support.

@ennru ennru mentioned this pull request Sep 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patriknw patriknw patriknw left review comments

@johanandren johanandren johanandren left review comments

@ennru ennru ennru approved these changes

Assignees
No one assigned
Labels
dependency-change For PRs changing the version of a dependency. documentation p:amqp p:aws-event-bridge p:aws-lambda p:azure-storage-queue p:cassandra p:elasticsearch p:ftp p:geode p:google-cloud-bigquery p:google-cloud-bigquery-storage p:google-cloud-pub-sub p:google-cloud-pub-sub-grpc p:google-cloud-storage p:google-common p:hbase p:huawei-push-kit p:reference
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Arikuti @ennru @patriknw @johanandren