search

bitemyapp / esqueleto

New home of Esqueleto, please file issues so we can get things caught up!
BSD 3-Clause "New" or "Revised" License
372 stars 107 forks source link

Unify ToFrom+ToFromT, ToAlias+ToAliasT and ToAliasReference+ToAliasReferenceT #211

Closed arthurxavierx closed 3 years ago

arthurxavierx commented 3 years ago

It would be nice if ToFromT, ToAliasT and ToAliasReferenceT were associated type families of ToFrom, ToAliasandToAliasReference`, respectively. That would let users implement custom instances of those type classes for cases where custom SQL expr types are used, or for cases where the standard tuple sizes supplied are not enough.

From a quick look into the source code it seems that this should be possible. Are there any objections against that?

belevy commented 3 years ago

So ToFromT is a closed type family to support the type errors. If you can come up with a better way to handle this and provide the errors I'm all ears, this was my first foray into Type Families and TypeError so I'm sure it could be much better. As for the other type families they probably could be opened and associated with the type classes, I closed them to be consistent with ToFromT.

As far as increasing tuple sizes, 8 was arbitrary and based off of boredom. It would probably make a ton of sense to just make a TH helper for generating the instances.

arthurxavierx commented 3 years ago

So ToFromT is a closed type family to support the type errors.

Ah yes, I noticed that when trying to code this haha

As for the other two type families I will open a PR that merges them into their corresponding type classes, if that's ok. That would solve a real use case where we have custom types in our DB layer with SqlSelect instances.

Thanks for the awesome work! :)

belevy commented 3 years ago

I would be okay opening them for that use case. But like I said above I feel like n-tuples would be best handled in the library itself. As a warning #215 causes a fair amount of changes to the Experimental module though I believe that only the ToFromT type family changed. On reflection ToFromT is probably doing too much and likely the type families as a whole can be simplified.

arthurxavierx commented 3 years ago

But like I said above I feel like n-tuples would be best handled in the library itself.

I agree. My use case is for custom data types actually, not for n-tuples (where n > 8).

As a warning #215 causes a fair amount of changes to the Experimental module though I believe that only the ToFromT type family changed.

Oh wow, that PR is wonderful! Yeah, I didn't cause any conflicts.

On another note, is there any reason why we need ToAliasT and ToAliasReferenceT? It seems that for all cases we have ToAliasT a ~ a and ToAliasReferenceT a ~ a.

belevy commented 3 years ago

There must have been a reason. But for the life of me I can't remember.

Thinking it through the aliased value/entity should always be exactly the same type. Semantically it would make no sense for them to be different. I solved subqueries very early on and never really revisited them.

If I had to guess at my reasoning, it was perhaps a misguided attempt at ensuring I aliased values in the processing of subqueries. Since the compiler can't guarantee that ToAliasReferenceT a ~ a.