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

Add ZIO module providing connect and transact mechanisms adapted to ZIO #47

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

guizmaii
Copy link
Contributor

@guizmaii guizmaii commented Oct 9, 2024

TODO:

  • Add tests

@guizmaii guizmaii force-pushed the zio_support branch 8 times, most recently from 7c3d2f9 to 269a093 Compare October 9, 2024 02:50
@AugustNagro AugustNagro mentioned this pull request Oct 10, 2024
@guizmaii guizmaii force-pushed the zio_support branch 2 times, most recently from 48bc71d to b6b2d7b Compare November 14, 2024 07:51
@guizmaii guizmaii marked this pull request as ready for review November 14, 2024 07:53
@guizmaii
Copy link
Contributor Author

@AugustNagro I finally added the tests. Sorry for the delay. Ready for review 🙂

@@ -0,0 +1,128 @@
package com.augustnagro.magnum.zio_integration

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

Copy link
Contributor Author

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

What about magzio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 🙂

Copy link
Contributor Author

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

Copy link

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))

Copy link
Contributor Author

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 :)

@guizmaii guizmaii force-pushed the zio_support branch 2 times, most recently from b496cd9 to 024ff1a Compare December 4, 2024 11:17

class PgZioTests extends FunSuite, TestContainersFixtures:

immutableRepoZioTests(this, PostgresDbType, xa)
Copy link
Contributor Author

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 🤔

@AugustNagro
Copy link
Owner

Awesome work thanks again @guizmaii .

@AugustNagro AugustNagro merged commit 835861c into AugustNagro:master Dec 5, 2024
1 check passed
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.

3 participants