softwaremill / sttp

The Scala HTTP client you always wanted!
https://sttp.softwaremill.com
Apache License 2.0
1.43k stars 299 forks source link

Add scala3 support for spray json #2181

Closed MichelEdkrantz closed 1 month ago

MichelEdkrantz commented 1 month ago

Before submitting pull request:

Tests could not complete because of OOM issues. Just ran this with standard settings on an M3 Macbook. Any common tips or tricks used here?

Running sbtformat yielded format changed for files i did not touch.

modified:   core/src/main/scala/sttp/client4/testing/ResponseStub.scala
    modified:   observability/opentelemetry-metrics-backend/src/main/scala/sttp/client4/opentelemetry/OpenTelemetryMetricsBackend.scala
    modified:   observability/opentelemetry-metrics-backend/src/main/scala/sttp/client4/opentelemetry/OpenTelemetryMetricsConfig.scala
    modified:   observability/prometheus-backend/src/main/scala/sttp/client4/prometheus/PrometheusBackend.scala
MichelEdkrantz commented 1 month ago

Discussed here #2179

adamw commented 1 month ago

Thanks!

Testing is a problem, due to Scala.JS+Chrome, which doesn't release memory or something like that. It's a problem known for a couple of years without a resolution ;) There's a work-around in the readme, bascially you need to run only a subset of tests:

Note that running the default test task will run the tests using both the JVM and JS backends, and is likely to run out of memory. If you'd like to run the tests using only the JVM backend, execute: sbt rootJVM/test.

But if the code compiles, that 90% of success ;)

As for formatting - yes, we should get an additional build step which verifies that files are properly formatted, although nobody got to it yet ;)

adamw commented 1 month ago

It seems there's a compilation error:

[error] -- [E172] Type Error: /home/runner/work/sttp/sttp/json/spray-json/src/test/scala/sttp/client4/SprayJsonTests.scala:85:105 
[error] 85 |    val request: Request[Either[String, String]] = basicRequest.get(Uri("http://example.org/")).body(json)
[error]    |                                                                                                         ^
[error]    |No given instance of type sttp.client4.BodySerializer[spray.json.JsObject] was found for a context parameter of method body in trait PartialRequestExtensions.
[error]    |I found:
[error]    |
[error]    |    sttp.client4.sprayJson.sprayBodySerializer[B]()
[error]    |
[error]    |But method sprayBodySerializer in trait SttpSprayJsonApi does not match type sttp.client4.BodySerializer[spray.json.JsObject].
[error] one error found
MichelEdkrantz commented 1 month ago

Hi, sorry about the initial state and thanks for the patience. This was a good learning experience for me, having very little experience with both sbt and cross-builds. It was not obvious to me at first that scala3 did not run locally.

It seems, this can be resolved by adding this implicit for scala3

implicit private val jsObjectSerializer: BodySerializer[JsObject] = sprayBodySerializer(RootJsObjectFormat)

This build is now ✅ https://github.com/MichelEdkrantz/sttp/actions/runs/9137873805

I did another branch forking from 3.9.6 for the client3, without these issues. https://github.com/MichelEdkrantz/sttp/actions/runs/9128354147

adamw commented 1 month ago

scala3 should run locally :) Are you compiling from the sbt console? It should be aggregated in the main build, but you can also switch projects (when in sbt console) by typing project sprayJson3 and then compile or test - to test only that simple project, using scala 3

MichelEdkrantz commented 1 month ago

Thanks, yes, figured out how to run it locally with Scala 3 in the sbt console now

adamw commented 1 month ago

Ok, great, maybe we do need some better docs on how to do this :)