karamaan / karamaan-opaleye

Other
11 stars 7 forks source link

ShowConstant instances for newtypes #50

Closed bergmark closed 10 years ago

bergmark commented 10 years ago

I wrote these helpers for defining ShowConstant instances:

showThrough1 :: forall a b. (ShowConstant a a) => (b -> a) -> b -> Expr (Wire b)
showThrough1 f = fmap unsafeCoerce . (showConstant :: a -> Expr (Wire a)) . f

showThrough2 :: forall a b. (Newtype b a, ShowConstant a a) => b -> Expr (Wire b)
showThrough2 = fmap unsafeCoerce . (showConstant :: a -> Expr (Wire a)) . unpack

Neither of them actually guarantee that we have a newtype (i.e. that unsafeCoerce is safe), and I also don't like Control.Newtype since it exposes the constructor.

Still, it's better than copy pasting the implementations from ShowConstant.hs.

Any ideas to improve this?

bergmark commented 10 years ago

Perhaps a bit overkill, but we could use TH to remove the unsafety.

bergmark commented 10 years ago

Hmmm was I thinking about this wrong? Are these actually safe?

tomjaguarpaw commented 10 years ago

What do you mean by "safe" in this context?

bergmark commented 10 years ago

e.g is this a valid instance (assuming we want to store Chars in int columns)?

instance ShowConstant Char Char where
  showConstant = fmap unsafeCoerce . (showConstant :: Int -> Expr (Wire Int)) . ord
tomjaguarpaw commented 10 years ago

Yes it looks like a reasonable instance to me, as long as you want to represent Chars as integers inside the database.

bergmark commented 10 years ago

Ok, great. unsafe* got me worried :-)

Do you think it's a good idea to include showThrough1 (with a better name)?

tomjaguarpaw commented 10 years ago

Wire.unsafeCoerce is "unsafe" in the Postgres sense, not the Haskell sense, i.e. it can lead to Postgres rejecting the query but can't lead to a Haskell crash. It would be unsafe if you created two different instances, one which represents Wire Char as Postgres integers and one which uses Postgres strings, say. Then you'll get Postgres type mismatches when you try to mix them.

tomjaguarpaw commented 10 years ago

Including something like showThrough1 sounds like a fine idea. I guess it would be used something like

newtype MyType = MyType { fromMyType :: Int }
instance ShowConstant MyType MyType where
    showConstant = showThrough1 fromMyType

We could have a little bit of TH to remove this boilerplate too, or maybe just some simple generics.

bergmark commented 10 years ago

I have this now:

newtype Id = IdX { unId :: Int }

newtypeColumn ''Id

======>

instance ShowConstant Id Id where
  showConstant = showThrough (\ (IdX x_aUU9) -> x_aUU9)

instance Default QueryRunner (Wire Id) Id where
  def = fieldQueryRunnerF IdX
tomjaguarpaw commented 10 years ago

Looks good. The Default QueryRunner instance is already done by this though:

https://github.com/karamaan/karamaan-opaleye/blob/master/Karamaan/Opaleye/RunQuery/TH.hs

bergmark commented 10 years ago

Ah I forgot that existed!