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

Scala native support #289

Merged
merged 17 commits into from
Jul 7, 2022
Merged

Scala native support #289

merged 17 commits into from
Jul 7, 2022

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Jul 1, 2022

Adds Scala Native support (for Scala 3 only, to avoid the matrix increasing in size too much for something that is quite bleeding edge).

The supported modules are :

  • smithy4s-core
  • smithy4s-json
  • smithy4s-dynamic

This PR also amends the dynamic module slightly to make it depend on the smithy-model library (on JVM only), facilitating the loading of dynamic services/schemas directly from a smithy Model. This makes tests a little bit easier to write and reduces the need for sharing some test-utilities across modules, which was leading to a cyclic module dependency that I was too lazy to solve via yet another indirection.

Additionally, because smithy4s-dynamic is built on native now, I had to remove the dependency to CE/weaver in its test. I wanted to keep the diff smallish, so I ended up making a dummy IO construct (totally not pure) to reduce the noise a little. When CE builds on native, we'll switch back.

@Baccata Baccata requested a review from kubukoz July 4, 2022 14:24
}
}

object Http4sDynamicPizzaClientSpec extends smithy4s.tests.PizzaClientSpec {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this one to the http4s module.

package smithy4s.dynamic
import smithy4s.tests._

object DummyIO {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

They call me Haxxor, destroyer of common-sense.

Seriously though, it was easier to do that than turn everything into a scala.util.Try

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhaha that's clever

case None => (map, Left(KeyNotFoundError(key)))
case Some(value) => (map - value, Right(()))
}
class KVStoreImpl(map: scala.collection.mutable.Map[String, String])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sue me.

build.sbt Show resolved Hide resolved
Ref[IO]
.of(Map.empty[String, String])
def kvStoreResource: IO[KVStore[IO]] = {
IO(scala.collection.mutable.Map.empty[String, String])
.map { ref =>
Copy link
Contributor

Choose a reason for hiding this comment

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

ref isn't an accurate name anymore more like mutableMap, go nameless

@daddykotex
Copy link
Contributor

Just asking, would you accept a PR where I update the copyright everywhere, that we would merge to main (and eventually here) so that we reduce the noise in this PR ?

@Baccata
Copy link
Contributor Author

Baccata commented Jul 5, 2022

I would certainly

@daddykotex
Copy link
Contributor

I would certainly

see #292

@Baccata Baccata marked this pull request as ready for review July 7, 2022 09:38
val munitDeps = Def.setting {
if (virtualAxes.value.contains(VirtualAxis.native)) {
Seq(
Dependencies.MunitMilestone.core.value % Test,
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reason why we can't switch to the Milestone all together

Copy link
Contributor

@daddykotex daddykotex left a comment

Choose a reason for hiding this comment

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

I'm ok with this PR.

I had one question regarding Munit but it's not a blocker and could be resolved w/ a comment I believe

@@ -17,11 +17,11 @@
package smithy4s

import smithy.api.TimestampFormat
import smithy4s.Timestamp._
import java.time._
// import smithy4s.Timestamp._
Copy link
Contributor

Choose a reason for hiding this comment

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

can we get rid of the comment here?

"js"
} else "jvm"
} else if (projectId.endsWith(nativeSuffix)) {
projectId = projectId.dropRight(nativeSuffix.length)
Copy link
Contributor

Choose a reason for hiding this comment

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

gotta love the mutation :D

@Baccata Baccata merged commit 49c9170 into main Jul 7, 2022
@Baccata Baccata deleted the scala-native branch July 7, 2022 16: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.

2 participants