Destroy all GADTs; Removes the From GADT and SqlExpr GADT

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

Destroy all GADTs; Removes the From GADT and SqlExpr GADT #228

Closed belevy closed 3 years ago

belevy commented 3 years ago

Remove From GADT. Renamed ToFrom to From and ToFromT to FromT.

There are a couple of orphan instances caused by this. It might make sense to move subqueries into the same module as the class. That orphan is basically the only one that can't be eliminated. The orphans caused by the different joins can be rectified.

@parsonsmatt Do you have feelings on this change. Better, worse, or the same?

Before submitting your PR, check that you've:

[x] Bumped the version number.
[x] Documented new APIs with Haddock markup.
[x] Added @since declarations to the Haddock.
[x] Ran stylish-haskell and otherwise adhered to the style guide.

After submitting your PR:

[x] Update the Changelog.md file with a link to your PR.
[ ] Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts).

parsonsmatt commented 3 years ago

yeah this rules!

GADTs are neat but simple type classes are even better :+1:

parsonsmatt commented 3 years ago

This is a breaking change to the internals of experimental, so I'm cool with calling it 3.4.1.0. 3.5.0.0 would be reasonable too.

belevy commented 3 years ago

This PR is relevant to #231. I went ahead and put it into what i think is a mergeable state. The checks failed even installing ghc into the action. @parsonsmatt any idea why?

parsonsmatt commented 3 years ago

fixed the CI stuff in #233 , merge that in and try again?

belevy commented 3 years ago

Paging @parsonsmatt I don't want to merge this in without your consent.

parsonsmatt commented 3 years ago

Can you do a quick self-review? It's hard to tell what the functional changes are vs the "moving code around" changes are.

belevy commented 3 years ago

This now includes the changes to SqlExpr

parsonsmatt commented 3 years ago

hell yeah buddy

belevy commented 3 years ago

the new functions probably need to get documentation added. Documentation needs to be updated to use the functions instead of the data types.

parsonsmatt commented 3 years ago

going to get my work for 2.12 in

then i want to merge the 2.12 support in, then try it on the mercury codebase

should be sweet

belevy commented 3 years ago

@parsonsmatt Got your 2.13 changes merged in, mind testing on mercurys code base

parsonsmatt commented 3 years ago

bles't thx

parsonsmatt commented 3 years ago

Just tested on the Mercury codebase. No errors or warnings or anything. Nice.

@hdgarrood @arthurxavierx would one of y'all be willing to test this on the Lumi codebase?

@eborden any chance y'all could try this out at Freckle?

hdgarrood commented 3 years ago

IIRC we are matching on SqlExpr constructors in a few places (I know, I know) so I not sure how long it will take me to get around to trying this out, but I'll try to fit it in at some point.

eborden commented 3 years ago

@parsonsmatt I'll pass this along.

codedmart commented 3 years ago

@parsonsmatt @belevy this PR compiles fine on Lumi's code. There were warnings on SqlQuery and Table for deprecations as expected, but were simple to update.

Has something changed for the outputted SQL because I ran into a runtime error with a query. I logged the SQL with esqueleto 3.4.2.1 and then also logged the SQL with this PR. Here is the problem error and problem diff: SqlError {sqlState = "42703", sqlExecStatus = FatalError, sqlErrorMsg = "column q3.id does not exist", sqlErrorDetail = "", sqlErrorHint = ""}

- "q3"."v11_id",         -- esqueleto 3.4.2.1
+ "q3"."id" AS "v17_id", -- esqueleto 3.5.0.0 (this PR)

belevy commented 3 years ago

@codedmart Are you able to post the query code? To answer your question, the query generation code has been shuffled around pretty hard and there is very possibly a bug with subqueries.

belevy commented 3 years ago

@codedmart The only thing i could figure was different was the realiasing of already referenced things, in the old code this was handled as a passthrough. I have updated to match that, would you be able to retest?

belevy commented 3 years ago

@codedmart and I were able to work out the issue. @parsonsmatt do we wait on Freckle?

parsonsmatt commented 3 years ago

@belevy I just fixed a bug with composite primary keys in a group by. Can you resolve merge conflicts?

I'm happy to merge it in today or tomorrow and do the release with deprecations. @eborden if you want some more time to do testing please let us know.

belevy commented 3 years ago

@parsonsmatt Change is merged in but it appears to be obviated by the change in representation of composite keys. Also fixed an issue in the SQLite tests where it was accidentally set to focus on the sqlite specific cases only. I'm cool with making 3.5 "A New Hope" and adding the deprecation warnings for the old syntax.

parsonsmatt commented 3 years ago

Man I gotta have the focus set of families fail CI

parsonsmatt commented 3 years ago

thanks @belevy for your patience! let's get this going 😎