You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/05/24 11:15:37 UTC
[GitHub] [incubator-pekko-http] mdedetrich opened a new pull request, #150: Scala 3 simple merge
mdedetrich opened a new pull request, #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206636339
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -235,7 +239,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
Post("/metrics", entity = data) ~> route ~> check {
status should ===(StatusCodes.OK)
- responseAs[String] should ===("""{"msg":"Total metrics received: 2"}""")
+ responseAs[String] should ===("Total metrics received: 2")
Review Comment:
Pinging @raboof
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206669662
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/RouteDirectivesExamplesSpec.scala:
##########
Review Comment:
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203959797
##########
project/Dependencies.scala:
##########
@@ -28,15 +28,16 @@ object Dependencies {
val h2specExe = "h2spec" + DependencyHelpers.exeIfWindows
val h2specUrl = s"https://github.com/summerwind/h2spec/releases/download/v${h2specVersion}/${h2specName}.zip"
- val scalaTestVersion = "3.1.4"
+ val scalaTestVersion = "3.2.9"
val specs2Version = "4.10.6"
- val scalaCheckVersion = "1.14.3"
+ val scalaCheckVersion = "1.15.4"
val scalafixVersion = _root_.scalafix.sbt.BuildInfo.scalafixVersion // grab from plugin
val scala212Version = "2.12.17"
val scala213Version = "2.13.10"
- val allScalaVersions = Seq(scala213Version, scala212Version)
+ val scala3Version = "3.2.2"
Review Comment:
I would prefer to do this in a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204042177
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/RouteDirectivesExamplesSpec.scala:
##########
Review Comment:
This entire file change is suspicious
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206651755
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -218,7 +222,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
.runFold(0) { (cnt, _) => cnt + 1 }
complete {
- measurementsSubmitted.map(n => Map("msg" -> s"""Total metrics received: $n"""))
+ measurementsSubmitted.map(n => s"""Total metrics received: $n""")
Review Comment:
I wouldn't be surprised if this was since solved in later Scala versions, there is some merit in spending a low amount of time to figure out if its solved but I will look into this when a future PR updating pekko to Scala 3.3 LTS is out.
Thanks for spending time looking into it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#issuecomment-1564271589
@pjfanning @He-Pin @raboof So I think the PR is ready to review, it turns out that none of the issues were really suspicious but changes legitimately necessary for Scala 3.
Regarding Scala 3.3, I changed my mind and I will try and update it in this PR if its trivial to do so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204041385
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/MarshallingDirectivesExamplesSpec.scala:
##########
@@ -53,6 +54,7 @@ class MarshallingDirectivesExamplesSpec extends RoutingSpec with CompileOnlySpec
"example-entity-with-raw-json" in {
// #example-entity-with-raw-json
+ import spray.json.JsValue
Review Comment:
Another suspicious change albeit this one seems harmless
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206631736
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -235,7 +239,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
Post("/metrics", entity = data) ~> route ~> check {
status should ===(StatusCodes.OK)
- responseAs[String] should ===("""{"msg":"Total metrics received: 2"}""")
+ responseAs[String] should ===("Total metrics received: 2")
Review Comment:
this does seem like an important change - I think we need to understand why this needs to change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206609146
##########
http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/SimpleHeaders.scala:
##########
@@ -111,14 +112,14 @@ private[parser] trait SimpleHeaders { this: Parser with CommonRules with CommonA
// http://tools.ietf.org/html/rfc7233#section-4.2
def `content-range` = rule {
- (`byte-content-range` | `other-content-range`) ~ EOI ~> (`Content-Range`(_, _))
+ `byte-content-range` ~ EOI ~> (`Content-Range`(_, _)) | `other-content-range` ~ EOI ~> (`Content-Range`(_, _))
Review Comment:
this change was part of 0a979b87f0 and indeed back then (on scala 3.1.3) was required to compile. The old syntax seems to work again on scala 3.2.2 and 3.3.0, though, so this change could be reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206637535
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -218,7 +222,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
.runFold(0) { (cnt, _) => cnt + 1 }
complete {
- measurementsSubmitted.map(n => Map("msg" -> s"""Total metrics received: $n"""))
+ measurementsSubmitted.map(n => s"""Total metrics received: $n""")
Review Comment:
This change was part of 63b3437048. It might indicate a change in behavior, but likely it's a change in behavior between Scala versions rather than between before and after the changes to the module. Since the `Map` to json encoding doesn't seem super relevant to this particular test it seemed more productive to just simplify it to a regular string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203972908
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -235,7 +239,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
Post("/metrics", entity = data) ~> route ~> check {
status should ===(StatusCodes.OK)
- responseAs[String] should ===("""{"msg":"Total metrics received: 2"}""")
+ responseAs[String] should ===("Total metrics received: 2")
Review Comment:
Another suspicious change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203962850
##########
parsing/src/main/scala/org/apache/pekko/macros/LogHelper.scala:
##########
@@ -13,11 +13,8 @@
package org.apache.pekko.macros
-import org.apache.pekko
-import pekko.annotation.InternalApi
-import pekko.event.LoggingAdapter
-
-import scala.reflect.macros.blackbox
+import org.apache.pekko.annotation.InternalApi
Review Comment:
maybe fix this import - move with rest of pekko ones and remove org.apache bit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206644757
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -218,7 +222,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
.runFold(0) { (cnt, _) => cnt + 1 }
complete {
- measurementsSubmitted.map(n => Map("msg" -> s"""Total metrics received: $n"""))
+ measurementsSubmitted.map(n => s"""Total metrics received: $n""")
Review Comment:
Yes - specifically in having the implicits correctly pick up that the `Map` can automatically be converted to a Json object and be Marshallable. It's not useless to figure out and show how to make that work again, but the risk that this is a bug that needs to be fixed rather than just something that would need a change in the test code to work on Scala 3 feels relatively low.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206647548
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/Rendering.scala:
##########
@@ -232,7 +233,7 @@ private[http] trait Rendering {
/**
* Renders the given string in double quotes.
*/
- def ~~#!(s: String): this.type = ~~('"').putEscaped(s) ~~ '"'
+ def ~~#!(s: String): this.type = this.~~('"').putEscaped(s, Rendering.`\"`, '\\').~~('"')
Review Comment:
So far all changes seem motivated and not introduced while dealing with the merge conflicts, but I'll verify a couple more to increase our confidence they are all legitimate...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204037910
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingFullExamples.scala:
##########
@@ -41,21 +42,21 @@ class JsonStreamingFullExamples extends AnyWordSpec {
import spray.json._
- implicit val userFormat = jsonFormat2(User)
+ implicit val userFormat: JsonFormat[User] = jsonFormat2(User.apply)
val `vnd.example.api.v1+json` =
MediaType.applicationWithFixedCharset("vnd.example.api.v1+json", HttpCharsets.`UTF-8`)
val ct = ContentType.apply(`vnd.example.api.v1+json`)
implicit def userMarshaller: ToEntityMarshaller[User] = Marshaller.oneOf(
- Marshaller.withFixedContentType(`vnd.example.api.v1+json`) { organisation =>
- HttpEntity(`vnd.example.api.v1+json`, organisation.toJson.compactPrint)
+ Marshaller.withFixedContentType(`vnd.example.api.v1+json`) { (user: User) =>
Review Comment:
Another suspicious change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#issuecomment-1564308271
> Looks like this breaks the scalafix build setup, but that seems worth it IMHO.
Indeed I will make an issue on this and we can solve it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203970460
##########
parsing/src/main/scala/org/apache/pekko/macros/LogHelper.scala:
##########
@@ -13,11 +13,8 @@
package org.apache.pekko.macros
-import org.apache.pekko
-import pekko.annotation.InternalApi
-import pekko.event.LoggingAdapter
-
-import scala.reflect.macros.blackbox
+import org.apache.pekko.annotation.InternalApi
Review Comment:
Fixed and pushed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203975122
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/RouteDirectivesExamplesSpec.scala:
##########
Review Comment:
Changes in this entire file are suspicious
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204249991
##########
docs/src/test/scala/docs/http/scaladsl/HttpClientExampleSpec.scala:
##########
@@ -318,9 +323,10 @@ class HttpClientExampleSpec extends AnyWordSpec with Matchers with CompileOnlySp
import pekko.http.scaladsl.unmarshalling.Unmarshal
import pekko.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._
import spray.json.DefaultJsonProtocol._
+ import spray.json.RootJsonFormat
case class Pet(name: String)
- implicit val petFormat = jsonFormat1(Pet)
+ implicit val petFormat: RootJsonFormat[Pet] = jsonFormat1(Pet.apply)
Review Comment:
it might be worth having a test in scala-2 specific test source that ensure that `implicit val petFormat = jsonFormat1(Pet)` still works for Scala 2 users - we can log this as a separate issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203971444
##########
project/Doc.scala:
##########
@@ -76,7 +76,13 @@ object Scaladoc extends AutoPlugin {
// Workaround https://issues.scala-lang.org/browse/SI-10028
"-skip-packages", "org.apache.pekko.pattern:org.specs2",
"-doc-canonical-base-url", "https://pekko.apache.org/api/pekko-http/current/") ++
- plugins.map(plugin => "-Xplugin:" + plugin)
+ plugins.map(plugin => "-Xplugin:" + plugin) ++
+ // Workaround https://issues.scala-lang.org/browse/SI-10028
+ (if (scalaBinaryVersion == "3")
+ // https://github.com/lampepfl/dotty/issues/14939
+ List("-skip-packages:akka.pattern:org.specs2")
Review Comment:
Fixed and pushed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204278640
##########
project/Dependencies.scala:
##########
@@ -45,11 +46,19 @@ object Dependencies {
object Provided {
val jsr305 = "com.google.code.findbugs" % "jsr305" % "3.0.2" % "provided" // ApacheV2
- val scalaReflect = ScalaVersionDependentModuleID.versioned("org.scala-lang" % "scala-reflect" % _ % "provided") // Scala License
+ val scalaReflect = ScalaVersionDependentModuleID.fromPF {
+ case v if v.startsWith("2.") => "org.scala-lang" % "scala-reflect" % v % "provided" // Scala License
+ }
}
object Compile {
- val scalaXml = "org.scala-lang.modules" %% "scala-xml" % "1.3.0" // Scala License
+ val scalaXml = {
+ val xml = "org.scala-lang.modules" %% "scala-xml" // Scala License
Review Comment:
since we are doing a v1.0.0 release and we can afford to have some changes - would it make sense to just upgrade and use the newer jar versions for all Scala versions? like this PR does for scalatest jar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206657379
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -218,7 +222,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
.runFold(0) { (cnt, _) => cnt + 1 }
complete {
- measurementsSubmitted.map(n => Map("msg" -> s"""Total metrics received: $n"""))
+ measurementsSubmitted.map(n => s"""Total metrics received: $n""")
Review Comment:
I checked that "just" switching back to a map is not sufficient even on 3.3.0. Might be just a matter of bringing into scope the right implicit, but I don't think I'll dive into that now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#issuecomment-1564279191
I've been reviewing this and it seems ready to go. I'll approve when the scala 3.3 change is in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#issuecomment-1561194041
Okay so I have managed to fix the issues with the merge conflicts. I did however find some highly suspicious changes, basically
I managed to find some suspicious changes and the reason why they are suspicious is that they seem to be changing actual business logic rather than the mechanical changes. All of these changes seem to stem from this commit https://github.com/apache/incubator-pekko-http/pull/150/commits/fb224dfba74d03d88866d5c40d63ebd9729cdded which so I think there may have been some errors in the merge commit.
@raboof Can you confirm this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204192203
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/RouteDirectivesExamplesSpec.scala:
##########
Review Comment:
Entire file change is suspicious
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203958126
##########
http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/SimpleHeaders.scala:
##########
@@ -111,14 +112,14 @@ private[parser] trait SimpleHeaders { this: Parser with CommonRules with CommonA
// http://tools.ietf.org/html/rfc7233#section-4.2
def `content-range` = rule {
- (`byte-content-range` | `other-content-range`) ~ EOI ~> (`Content-Range`(_, _))
+ `byte-content-range` ~ EOI ~> (`Content-Range`(_, _)) | `other-content-range` ~ EOI ~> (`Content-Range`(_, _))
Review Comment:
This change is somewhat suspicious, let me look into it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203958835
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/Rendering.scala:
##########
@@ -232,7 +233,7 @@ private[http] trait Rendering {
/**
* Renders the given string in double quotes.
*/
- def ~~#!(s: String): this.type = ~~('"').putEscaped(s) ~~ '"'
+ def ~~#!(s: String): this.type = this.~~('"').putEscaped(s, Rendering.`\"`, '\\').~~('"')
Review Comment:
Another suspicious change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich merged pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#issuecomment-1560942306
Okay I need to clean this up, evidently something happened
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204325048
##########
project/Dependencies.scala:
##########
@@ -45,11 +46,19 @@ object Dependencies {
object Provided {
val jsr305 = "com.google.code.findbugs" % "jsr305" % "3.0.2" % "provided" // ApacheV2
- val scalaReflect = ScalaVersionDependentModuleID.versioned("org.scala-lang" % "scala-reflect" % _ % "provided") // Scala License
+ val scalaReflect = ScalaVersionDependentModuleID.fromPF {
+ case v if v.startsWith("2.") => "org.scala-lang" % "scala-reflect" % v % "provided" // Scala License
+ }
}
object Compile {
- val scalaXml = "org.scala-lang.modules" %% "scala-xml" % "1.3.0" // Scala License
+ val scalaXml = {
+ val xml = "org.scala-lang.modules" %% "scala-xml" // Scala License
Review Comment:
CI passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206640297
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -218,7 +222,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
.runFold(0) { (cnt, _) => cnt + 1 }
complete {
- measurementsSubmitted.map(n => Map("msg" -> s"""Total metrics received: $n"""))
+ measurementsSubmitted.map(n => s"""Total metrics received: $n""")
Review Comment:
Right, so if I understand correctly there probably was some issue with construction of `Map` between Scala 2 and Scala 3 and the solution was "screw it, lets just not use a `Map` because its not important and its just in tests anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206672717
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingFullExamples.scala:
##########
@@ -41,21 +42,21 @@ class JsonStreamingFullExamples extends AnyWordSpec {
import spray.json._
- implicit val userFormat = jsonFormat2(User)
+ implicit val userFormat: JsonFormat[User] = jsonFormat2(User.apply)
val `vnd.example.api.v1+json` =
MediaType.applicationWithFixedCharset("vnd.example.api.v1+json", HttpCharsets.`UTF-8`)
val ct = ContentType.apply(`vnd.example.api.v1+json`)
implicit def userMarshaller: ToEntityMarshaller[User] = Marshaller.oneOf(
- Marshaller.withFixedContentType(`vnd.example.api.v1+json`) { organisation =>
- HttpEntity(`vnd.example.api.v1+json`, organisation.toJson.compactPrint)
+ Marshaller.withFixedContentType(`vnd.example.api.v1+json`) { (user: User) =>
Review Comment:
Also part of 63b3437048, seems to be a case of scala3 not inferring the type of this parameter without it. Still seems to be the case for Scala 3.3.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#issuecomment-1564280420
Okay ill push the commit now, its a trivial one liner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203961023
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -218,7 +222,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
.runFold(0) { (cnt, _) => cnt + 1 }
complete {
- measurementsSubmitted.map(n => Map("msg" -> s"""Total metrics received: $n"""))
+ measurementsSubmitted.map(n => s"""Total metrics received: $n""")
Review Comment:
Another suspicious change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204278640
##########
project/Dependencies.scala:
##########
@@ -45,11 +46,19 @@ object Dependencies {
object Provided {
val jsr305 = "com.google.code.findbugs" % "jsr305" % "3.0.2" % "provided" // ApacheV2
- val scalaReflect = ScalaVersionDependentModuleID.versioned("org.scala-lang" % "scala-reflect" % _ % "provided") // Scala License
+ val scalaReflect = ScalaVersionDependentModuleID.fromPF {
+ case v if v.startsWith("2.") => "org.scala-lang" % "scala-reflect" % v % "provided" // Scala License
+ }
}
object Compile {
- val scalaXml = "org.scala-lang.modules" %% "scala-xml" % "1.3.0" // Scala License
+ val scalaXml = {
+ val xml = "org.scala-lang.modules" %% "scala-xml" // Scala License
Review Comment:
since we are doing a v1.0.0 release and we can afford to have some changes - would it make sense to just upgrade and use the newer jar versions for all Scala versions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204042177
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/RouteDirectivesExamplesSpec.scala:
##########
Review Comment:
This entire file change is suspicious
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206627450
##########
docs/src/test/scala/docs/http/scaladsl/HttpClientExampleSpec.scala:
##########
@@ -318,9 +323,10 @@ class HttpClientExampleSpec extends AnyWordSpec with Matchers with CompileOnlySp
import pekko.http.scaladsl.unmarshalling.Unmarshal
import pekko.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._
import spray.json.DefaultJsonProtocol._
+ import spray.json.RootJsonFormat
case class Pet(name: String)
- implicit val petFormat = jsonFormat1(Pet)
+ implicit val petFormat: RootJsonFormat[Pet] = jsonFormat1(Pet.apply)
Review Comment:
thinking about this again, this is spray-json code so it should be covered by spray-json tests - we probably don't really need to do extra testing in pekko-http
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206632794
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -235,7 +239,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
Post("/metrics", entity = data) ~> route ~> check {
status should ===(StatusCodes.OK)
- responseAs[String] should ===("""{"msg":"Total metrics received: 2"}""")
+ responseAs[String] should ===("Total metrics received: 2")
Review Comment:
Agreed, it basically does see like some test/business logic was updated and not related to scala 3 at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203927407
##########
.asf.yaml:
##########
@@ -35,11 +35,6 @@ github:
# contexts are the names of checks that must pass
contexts:
- Code is formatted
- - Check headers
- required_pull_request_reviews:
Review Comment:
We probably don't want to remove this
##########
build.sbt:
##########
@@ -11,7 +11,7 @@ import sbtdynver.GitDescribeOutput
import spray.boilerplate.BoilerplatePlugin
import com.lightbend.paradox.apidoc.ApidocPlugin.autoImport.apidocRootPackage
-sourceDistName := "incubating-pekko-http"
+sourceDistName := "incubator-pekko-http"
Review Comment:
we can't do this revert
##########
.github/workflows/nightly.yml:
##########
@@ -18,7 +18,7 @@ jobs:
PEKKO_VERSION: [default, main]
steps:
- name: Checkout
- uses: actions/checkout@v3
+ uses: actions/checkout@v2
Review Comment:
can we avoid this revert?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204099143
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/RouteDirectivesExamplesSpec.scala:
##########
Review Comment:
This entire file change is suspicious.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203960869
##########
project/Doc.scala:
##########
@@ -76,7 +76,13 @@ object Scaladoc extends AutoPlugin {
// Workaround https://issues.scala-lang.org/browse/SI-10028
"-skip-packages", "org.apache.pekko.pattern:org.specs2",
"-doc-canonical-base-url", "https://pekko.apache.org/api/pekko-http/current/") ++
- plugins.map(plugin => "-Xplugin:" + plugin)
+ plugins.map(plugin => "-Xplugin:" + plugin) ++
+ // Workaround https://issues.scala-lang.org/browse/SI-10028
+ (if (scalaBinaryVersion == "3")
+ // https://github.com/lampepfl/dotty/issues/14939
+ List("-skip-packages:akka.pattern:org.specs2")
Review Comment:
should be 'org.apache.pekko.pattern' - this 'akka.pattern' issue appears in `else` too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206668580
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/RouteDirectivesExamplesSpec.scala:
##########
Review Comment:
This change was part of 63b34370483aab3e10716e031d98c75c1bfc2e40. The `Content-Type` case class constructor is `private[http]`, because people are supposed to use the HttpEntity subclass to access it, rather than looking at the headers themselves. Apparently this was somehow not always correctly enforced on Scala 2.
On Scala 3 this appears to have been fixed, so the test wouldn't compile as-is anymore, and it seemed more 'realistic' to switch to a different header to test the route directives.
If it turns out there are legitimate use cases for constructing `Content-Type` header objects we might consider removing the `private[http]` from that constructor, but for now it seems conservative to keep it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #150: Scala 3 support
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1203958858
##########
project/Dependencies.scala:
##########
@@ -28,15 +28,16 @@ object Dependencies {
val h2specExe = "h2spec" + DependencyHelpers.exeIfWindows
val h2specUrl = s"https://github.com/summerwind/h2spec/releases/download/v${h2specVersion}/${h2specName}.zip"
- val scalaTestVersion = "3.1.4"
+ val scalaTestVersion = "3.2.9"
val specs2Version = "4.10.6"
- val scalaCheckVersion = "1.14.3"
+ val scalaCheckVersion = "1.15.4"
val scalafixVersion = _root_.scalafix.sbt.BuildInfo.scalafixVersion // grab from plugin
val scala212Version = "2.12.17"
val scala213Version = "2.13.10"
- val allScalaVersions = Seq(scala213Version, scala212Version)
+ val scala3Version = "3.2.2"
Review Comment:
will we go ahead and upgrade to Scala 3.3.0?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204099143
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/RouteDirectivesExamplesSpec.scala:
##########
Review Comment:
This entire file change is suspicious.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1204299157
##########
project/Dependencies.scala:
##########
@@ -45,11 +46,19 @@ object Dependencies {
object Provided {
val jsr305 = "com.google.code.findbugs" % "jsr305" % "3.0.2" % "provided" // ApacheV2
- val scalaReflect = ScalaVersionDependentModuleID.versioned("org.scala-lang" % "scala-reflect" % _ % "provided") // Scala License
+ val scalaReflect = ScalaVersionDependentModuleID.fromPF {
+ case v if v.startsWith("2.") => "org.scala-lang" % "scala-reflect" % v % "provided" // Scala License
+ }
}
object Compile {
- val scalaXml = "org.scala-lang.modules" %% "scala-xml" % "1.3.0" // Scala License
+ val scalaXml = {
+ val xml = "org.scala-lang.modules" %% "scala-xml" // Scala License
Review Comment:
iirc there was issues with scala-xml on JDK 8 but that is running in CI so lets see if it makes an impact
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206618522
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/Rendering.scala:
##########
@@ -232,7 +233,7 @@ private[http] trait Rendering {
/**
* Renders the given string in double quotes.
*/
- def ~~#!(s: String): this.type = ~~('"').putEscaped(s) ~~ '"'
+ def ~~#!(s: String): this.type = this.~~('"').putEscaped(s, Rendering.`\"`, '\\').~~('"')
Review Comment:
Thanks. Not sure if it makes sense to go through every comment that I marked as suspicious, I was just concerned whether a regression was made when dealing with the merge conflicts.
If you think all of the suspicious parts I marked are legitimate just let me know and we can finally review and merge the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206615058
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/Rendering.scala:
##########
@@ -232,7 +233,7 @@ private[http] trait Rendering {
/**
* Renders the given string in double quotes.
*/
- def ~~#!(s: String): this.type = ~~('"').putEscaped(s) ~~ '"'
+ def ~~#!(s: String): this.type = this.~~('"').putEscaped(s, Rendering.`\"`, '\\').~~('"')
Review Comment:
This change was part of ebd17d4dce and indeed still seems required to compile
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #150: Scala 3 support
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206640297
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -218,7 +222,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
.runFold(0) { (cnt, _) => cnt + 1 }
complete {
- measurementsSubmitted.map(n => Map("msg" -> s"""Total metrics received: $n"""))
+ measurementsSubmitted.map(n => s"""Total metrics received: $n""")
Review Comment:
Right, so if I understand correctly there probably was some issue with construction of `Map` between Scala 2 and Scala 3 and the solution was "screw it, lets just not use a `Map` because its not important and its just in tests anyways."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206638001
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/JsonStreamingExamplesSpec.scala:
##########
@@ -235,7 +239,7 @@ class JsonStreamingExamplesSpec extends RoutingSpec with CompileOnlySpec {
Post("/metrics", entity = data) ~> route ~> check {
status should ===(StatusCodes.OK)
- responseAs[String] should ===("""{"msg":"Total metrics received: 2"}""")
+ responseAs[String] should ===("Total metrics received: 2")
Review Comment:
(counterpart of https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206637535)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] raboof commented on a diff in pull request #150: Scala 3 support
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #150:
URL: https://github.com/apache/incubator-pekko-http/pull/150#discussion_r1206674292
##########
docs/src/test/scala/docs/http/scaladsl/server/directives/MarshallingDirectivesExamplesSpec.scala:
##########
@@ -53,6 +54,7 @@ class MarshallingDirectivesExamplesSpec extends RoutingSpec with CompileOnlySpec
"example-entity-with-raw-json" in {
// #example-entity-with-raw-json
+ import spray.json.JsValue
Review Comment:
Part of 63b3437048, likely not strictly needed but in general we try to include relevant imports in code snippets that end up in the docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org