-
Notifications
You must be signed in to change notification settings - Fork 108
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
Generate ToMaybe
in deriveEsqueletoRecord
#378
Generate ToMaybe
in deriveEsqueletoRecord
#378
Conversation
d348f6d
to
e88a28e
Compare
Opening this up for conversation since the draft didn't get eyes and I think this is nearly there. |
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.
Could you write a test that shows it correctly working with Nothing
values? ie where the DB gives us NULL
back?
test/Common/Record.hs
Outdated
@@ -8,25 +8,34 @@ | |||
{-# LANGUAGE MultiParamTypeClasses #-} | |||
{-# LANGUAGE NamedFieldPuns #-} | |||
{-# LANGUAGE OverloadedLabels #-} | |||
{-# LANGUAGE OverloadedRecordDot #-} |
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.
We probably need to gate this behind CPP for the GHC version since we support GHC that doesn't have this feature yet
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 assume gating the whole file makes sense here?
587a6d4
to
bed5a2c
Compare
bed5a2c
to
be6fee2
Compare
@parsonsmatt did you mean something different from: https://github.com/bitemyapp/esqueleto/pull/378/files#diff-bef0a2133965554f3b49d699a015a29f235548c9ae5edc52ce80cd348332720dR282-R300 |
Confirmed this compiles and all existing tests run and pass on 8.10.7 |
test/Common/Record.hs
Outdated
#else | ||
it "is only supported in GHC 9.2 or above" $ \_ -> do | ||
pending | ||
#endif |
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.
The feature is supported in GHC <9.2, but you do need to use getField @"blah"
explicitly instead of (.blah)
. So we need to gate use of OverloadedRecordDot
behind CPP to support older GHC, but not the entire feature/tests.
Actually, it is probably easier to just getField
directly? Then there's no CPP necessary
I don't have a machine I can run 8.6.5 on until November 4th so fixing this will have to wait until then unless someone else can do the update here. |
replaces #370
@since
declarations to the Haddock.stylish-haskell
and otherwise adhered to the style guide.After submitting your PR: