-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add ZIO module providing connect
and transact
mechanisms adapted to ZIO
#47
Conversation
7c3d2f9
to
269a093
Compare
48bc71d
to
b6b2d7b
Compare
@AugustNagro I finally added the tests. Sorry for the delay. Ready for review 🙂 |
bb9cfeb
to
413551d
Compare
@@ -0,0 +1,128 @@ | |||
package com.augustnagro.magnum.zio_integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please no underscore in package names. Either .integration.zio
or just .zio
I'd say. I prefer the latter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? The issue with naming it zio
is that it creates really annoying conflicts in the imports.
This code:
import com.augustnagro.magnum.zio
import zio.Task
will fail to compile because Task
is not in the com.augustnagro.magnum.zio
package.
It forces people to do this manually:
import com.augustnagro.magnum.zio
import _root_.zio.Task
Really not great UX.
I initially called it com.augustnagro.magnum.zio
but even just coding this PR it was annoying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.zio.integration
Underscore is against JVM package name convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about magzio
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests are failing. I'll have a look at that after work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did not copy the resource files into the zio module
val carSql =
Files.readString(Path.of(getClass.getResource("/pg-car.sql").toURI))
val personSql =
Files.readString(Path.of(getClass.getResource("/pg-person.sql").toURI))
val bigDecSql =
Files.readString(Path.of(getClass.getResource("/pg-bigdec.sql").toURI))
val noIdSql =
Files.readString(Path.of(getClass.getResource("/pg-no-id.sql").toURI))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did not copy the resource files into the zio module
I don't need as the zio
module depends on the magnum
module with "test->test"
I had to re-write the tests as they have changed :)
b496cd9
to
024ff1a
Compare
|
||
class PgZioTests extends FunSuite, TestContainersFixtures: | ||
|
||
immutableRepoZioTests(this, PostgresDbType, xa) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only re-implemented the immutableRepoTests
. Not sure if there's any value in re-implementing more of them 🤔
Awesome work thanks again @guizmaii . |
TODO: