-
Notifications
You must be signed in to change notification settings - Fork 32
Properly recurse into :*: #178
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
Conversation
Can confirm this solves the issue. Thanks for fixing it! |
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.
Looks good thank you!
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.
Can you please add a test that takes advantage of the fix in this code?
Will that do? If the test compiles it "should" work. Can I write the actual test somehow? |
Yes, please add a test. It should use a resource definition like the one you provided and it should call |
What's the status on this? |
This needs some kind of executable test before it can be merged. I've asked @dminuoso to do that, but they are a volunteer, so it's up to them when they do it. If you would like to create a new PR that includes the work of this one and adds the necessary tests, that would be very much appreciated. |
I'll look at it this week.
…On Mon, Jul 16, 2018, 08:13 Jonathan Lange ***@***.***> wrote:
This needs some kind of executable test before it can be merged. I've
asked @dminuoso <https://github.com/dminuoso> to do that, but they are a
volunteer, so it's up to them when they do it. If you would like to create
a new PR that includes the work of this one and adds the necessary tests,
that would be very much appreciated.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#178 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ATQbiQOJF8pRyd2DqqggQwq-HyG78XWLks5uHC8XgaJpZM4TJ3HM>
.
|
I have
I can avoid the error if I limit the number of fields to three. |
@EdmundsEcho: It looks like |
Sorry, work stuff kept me too occupied. The issue with writing tests here is that I'd have to write generics for the other direction from scratch. @Gabriel439 Do you have a testcase that I can use to validate my fix? |
@dminuoso: The following example fails to derive the {-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE DeriveGeneric #-}
import GHC.Generics (Generic)
import GraphQL.API (HasAnnotatedInputType)
data Example = Example
{ field0 :: Bool
, field1 :: Bool
, field2 :: Bool
, field3 :: Bool
} deriving (Generic, HasAnnotatedInputType) ... but if you remove one of the fields then the example works: {-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE DeriveGeneric #-}
import GHC.Generics (Generic)
import GraphQL.API (HasAnnotatedInputType)
data Example = Example
{ field0 :: Bool
, field1 :: Bool
, field2 :: Bool
} deriving (Generic, HasAnnotatedInputType) I tested those two examples using |
Yeah it's all addressed by 5747b88 |
The old FromValue code makes the assumption that product types are always of the shape
(a :*: (b :*: c))
.However GHC balances product and sum types (https://hackage.haskell.org/package/base-4.11.0.0/docs/GHC-Generics.html#g:9)
Resolves #173