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

Implement renderInsert for Oracle #695

Merged
merged 22 commits into from
Jul 2, 2022
Merged

Implement renderInsert for Oracle #695

merged 22 commits into from
Jul 2, 2022

Conversation

peixunzhang
Copy link
Contributor

No description provided.

@peixunzhang peixunzhang requested a review from a team as a code owner May 27, 2022 14:39
@peixunzhang
Copy link
Contributor Author

Hi, I don't if I should get rid of the duplications between the Postgres module and this one.

Copy link
Collaborator

@sviezypan sviezypan left a comment

Choose a reason for hiding this comment

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

Thanks for you work. Can you please also add few tests, something like in #685 ?

@sviezypan
Copy link
Collaborator

@peixunzhang seems that some tests have problems, can you please check?

Copy link
Collaborator

@sviezypan sviezypan left a comment

Choose a reason for hiding this comment

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

Looks good, just had a couple of questions.

@@ -55,6 +58,13 @@ trait JdbcRunnableSpec extends ZIOSpecDefault with Jdbc {
SqlDriver.live
)

protected implicit def genInstances[R]
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

durationCol
).values(row)

// TODO: ensure we can read values back correctly
Copy link
Collaborator

Choose a reason for hiding this comment

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

has this worked? test like this would be awesome!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
if I remember correctly there was some issue with reading the data back. Can't check right now because of m1 chip.
Long term we should definitely enable this test.

@jczuchnowski jczuchnowski merged commit 9e317ff into zio:master Jul 2, 2022
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.

4 participants