Skip to content
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

Fixes #340. Thunk results are completed as per non-thunk results. #341

Merged

Conversation

jjeffery
Copy link
Contributor

@jjeffery jjeffery commented Jun 28, 2018

Note that the following tests previously passed and have been modified.

  • TestLists_NullableListOfNonNullArrayOfFunc_ContainsNulls
  • TestLists_NonNullListOfNonNullArrayOfFunc_ContainsNulls

It seems to me that both of these tests should have failed previously, as they involved passing nil values where nil values were not permitted. Please advise if my understanding is incorrect.

…sults.

Note that the following tests previously passed and have been modified.

* TestLists_NullableListOfNonNullArrayOfFunc_ContainsNulls
* TestLists_NonNullListOfNonNullArrayOfFunc_ContainsNulls

It seems to me that both of these tests should have failed previously, as they involved passing nil values where nil values were not permitted.
@coveralls
Copy link

coveralls commented Jun 28, 2018

Coverage Status

Coverage increased (+0.03%) to 80.752% when pulling 7f9745e on jjeffery:issue-340-thunks-returning-objects into 18b9a05 on graphql-go:master.

Left a comment to include when support for go1.8 is dropped, as this is useful for identifying the real location of the error.
@jjeffery
Copy link
Contributor Author

Reading the GraphQL spec (June 2018), section 3.12.1.
The following entry applies to the TestLists_NullableListOfNonNullArrayOfFunc_ContainsNulls test:

[Int!] [1, 2, null] null (With logged coercion error)

This PR changes the test to return the result consistent with the GraphQL spec.

When it comes to the test TestLists_NonNullListOfNonNullArrayOfFunc_ContainsNulls, the relevant entry in section 3.12.1 is:

[Int!]! [1, 2, null] Error: Item cannot be null

This one might not be quite as compliant, but it is closer than the test prior to this PR.

Please advise if anyone has a different understanding.

@chris-ramon
Copy link
Member

LGTM 👍 — thanks a lot @jjeffery!
closes: #340

@chris-ramon chris-ramon merged commit 86f19d5 into graphql-go:master Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants