#211 Add 'make_*' functions to PostgresModule

[brbrown25]

No description provided.

[brbrown25]

• sync with master
• implement interval
• add interval test
• ensure tests are working

[brbrown25]

I'll be hacking away at this again today, got side tracked with the holidays!

[brbrown25]

@robmwalsh I've finally got this working, I'm thinking it's actually using the toString version of the Interval and Timestampz case classes which might be fine for now. Let me know any thoughts you have for improving and I'll address them. I added in the postgres driver to reference the PGInterval class, which is similar to how slick implements this functionality.

[bertlebee]

nice work - I don't have a problem with adding the postgres driver as I think we're going to move away from JDBC. A few more changes and this should be good to go. Let me know if you need a hand with any of it.

[brbrown25]

• Move Interval.scala from zio.sql.custom -> zio.sql.postgresql
• Create PostgresTypeTagExtension and move PG specifics into that
• Remove any Arrity FunctionCallX that isn't really used
• Double check which modules I'm adding the pg driver dependency to
• Timestampz should be using ZonedDateTime

[brbrown25]

@robmwalsh this is a little bit closer to what I think we'd like ultimately. I'm wondering What you think I need to either 1) just us the toString I've defined on Timestampz and Interval or 2) have it use the tupled form of them. Also I think you were suggesting I remove the Timestampz case class, given how I've structured this do you think that still makes sense. My main motivator was that it provides a nice interface for creating them that matches the function args of the postgres function. Excited to get this a little bit closer :)

[brbrown25]

@robmwalsh I have the question above about Timestampz and wondering if you have any ideas about how to fix my broken tests.

[bertlebee]

@brbrown25 @jczuchnowski we now have a working dialect specific type tag for postgres :)

The main issue was we were conflating type tags (which are associated with the values but are just implicit objects) with the values themselves. Interval didn't need to extend anything.

Thanks to @jczuchnowski for finding the cause of the compiling never ending.

@brbrown25 can you please pull the latest commits? your tests are now failing again :)
Interestingly there's an error involving the postgres library objects - I don't think we should be using PGInterval directly, just providing a way to convert to and from one to our Interval representation. I haven't actually looked at any of your test code though so could be something simple.

[brbrown25]

@brbrown25 @jczuchnowski we now have a working dialect specific type tag for postgres :)

The main issue was we were conflating type tags (which are associated with the values but are just implicit objects) with the values themselves. Interval didn't need to extend anything.

Thanks to @jczuchnowski for finding the cause of the compiling never ending.

@brbrown25 can you please pull the latest commits? your tests are now failing again :)

Interestingly there's an error involving the postgres library objects - I don't think we should be using PGInterval directly, just providing a way to convert to and from one to our Interval representation. I haven't actually looked at any of your test code though so could be something simple.

Thanks! Yeah I'll pull and look. The PGInterval should only be used when the decoding is happening but I'll get to the bottom of that!

[brbrown25]

• pull latest
• fix failing test

[brbrown25]

@robmwalsh @jczuchnowski I'm down to the following issues that I'm not quite sure how to address.

1. The make_timestamptz I want to render as SELECT make_timestamptz(2013, 7, 15, 8, 15, 23, '+00') but currently it's rendering as SELECT make_timestamptz('2013-07-15T08:15:23.5+00:00') which is it just stringing the ZonedDatetime. So I switched to just passing in a tuple of the args and that makes it pass for now. One issue with the ZonedDateTime is that you can't pass in seconds as a decimal so 23.5 is a valid value to the make_timestamptz for seconds, however ZonedDateTime wants everything as ints.
2. The interval test is now throwing

[info]     An unchecked error was produced.
[info]     scala.NotImplementedError: an implementation is missing

But I'm not quite sure where that's coming from.

[bertlebee]

very minor stuff now :) Please also do a final pass to make sure you remove any unneeded comments

[brbrown25]

@robmwalsh I've gone ahead and cleaned this up and squashed it down into a single commit. I really wanted to be able to do something like def decode[A, E <: DecodingError](column: Either[Int, String], resultSet: ResultSet): Either[E, A] = ??? for the definition of decode, but the compiler wouldn't let me. I also made one more change which was bringing back the Timestampz case class. Let me know any additional changes you'd like and I'll rock them out quickly. I should have the PGInterval pr in the next week or so.

[bertlebee]

Thanks, just some (hopefully) minor stuff now.

[bertlebee]

Using ??? is turning a possible compiler error "you need to implement def decode" into a possible runtime error (NotImplementedError), which we like to avoid where possible! We need to make sure every extension type is Decodable and that is has a valid decode implementation. If you drop the = ??? from the end, it'll let the compiler stop us making mistakes. In general, you should only ever use ??? to plug gaps temporarily; the implementation shows you why:

def ??? : Nothing = throw new NotImplementedError

Unrelated, I don't think you should be introducing new type parameters here - this is shadowing the +A type parameter in trait Decodable[+A], which means it can actually return anything instead of whatever the +A type in Decodable is (which I think is what we actually want). Similarly DecodingError is shadowing self.ReadExecutor.DecodingError, which is what we actually want to return here. How you can actually access that from here could be tricky.

[brbrown25]

• drop the ???

[brbrown25]

Yeah I agree about the not introducing new type parameters. We can talk in our pair about how to handle DecodingError The reason I kept the A was for Timstampz to let it return a ZonedDateTime instead of a Timestampz

[bertlebee]

boom!