-
Notifications
You must be signed in to change notification settings - Fork 35
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
Upgrade LTS, generic derivations, SQL quasi quoter #50
base: master
Are you sure you want to change the base?
Conversation
I moved `convertError` from `Database.MySQL.Simpe.QueryResults` into `Database.MySQL.Simple.Result` to avoid a cyclic dependency between the `Database.MySQL.Simpe.QueryResults` and `Database.MySQL.Simpe.QueryResults.Generic` that both make use of said function. Note that this change requires GHC Haskell. We may want to hide this behind a preprocessor flag for that reason. I don't quite know if it makes sense to derive instances for `U1` (types with a single constructor and no fields). Although it is certainly helpful during development to not get a type error when trying to derive instances for a type that you are in the process of defining, like: data User = User I have also noted a concern with my generic implementation for `(:*:)`. I'll reproduce it here: I'm concerned about this implementation since it's biased towards one side. Meaning that it's expecting the chain of `(:*:)` to lean to the right. As an example of the problem consider the following: > data T = T Int Int Int deriving (Generic) > from (T 1 2 3) M1 {unM1 = M1 {unM1 = M1 {unM1 = K1 {unK1 = 1}} :*: (M1 {unM1 = K1 {unK1 = 2}} :*: M1 {unM1 = K1 {unK1 = 3}})}} If the result in stead had been like this (note the re-bracketing): M1 {unM1 = (M1 {unM1 = M1 {unM1 = K1 {unK1 = 1}} :*: M1 {unM1 = K1 {unK1 = 2}}}) :*: M1 {unM1 = K1 {unK1 = 3}}} Then the generic derivation for `T` would fail. Furthermore, to be able to supply the arity in the call to `convertError` I think we might need a `Typeable (a :*: b)` constraint. Lastly, the existing implementations for `QueryResults` uses strictness annotations in certain places. I don't know if these are strictly (pun!) necessary and haven't used them in the generic implementation.
03d3e56
to
ee3bba3
Compare
Thanks - I think this is a good idea. Obviously it needs to be fixed to pass the tests on older LTS versions. |
I was wondering if you have any take on my concerns mentioned in the commit message of 2e1619f. |
Goodbye to these lovely GHC extensions :( * TypeApplications * KindSignatures Also turns on -Wall and -Werror in some modules.
735c3d5
to
2766106
Compare
EDITED
The "slots in the target type" are now being inferred generically. |
62c33c2
to
75ebd98
Compare
convertResults err (x:xs) (y:ys) = | ||
-- I'm concerned about this implementation since it's biased | ||
-- towards one side. Meaning that it's expecting the chain of | ||
-- ':*:' to lean to the right. As an example of the problem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear on how this all fits together, but that's an extremely bad assumption. Once there are more than three fields, the generic representation will be a tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I should look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't manage to describe the problem properly in this comment. The issue is not what the generic representation generated by from
is. The problem is the generic representation that we generate and then later try and convert back to some type using to
. Although the problem may be related since I suspect from
and to
are inverses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really figure out how to give a sensible definition for this. If someone wants to take a stab at it this snippet may come in handy for testing. https://gist.github.com/fredefox/79e8bcae045eada98934df88bdad7783
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a test-case for this.
Thanks for the comments @treeowl! I like your type error patch for |
…rity Credit to David Feuer.
c55f628
to
8ca2778
Compare
8ca2778
to
bc73b14
Compare
This change incorporate three changes
QueryParams
andQueryResult
Please see the relevant commit messages for more context.