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

Change resolve function missing error to warning #134

Merged
merged 8 commits into from
Sep 11, 2016

Conversation

nicolaslopezj
Copy link
Contributor

As talked in #131, resolve function missing validator is now showing a warning instead of an error

@DxCx
Copy link
Contributor

DxCx commented Sep 9, 2016

Hi @nicolaslopezj
Please make sure to update the test as well
And make sure that npm test yileds no errors :)

@nicolaslopezj
Copy link
Contributor Author

I'm not sure on what to do with the tests. Remove them? now the error is no longer required.

Errors:

1) throws an error if a field has arguments but no resolve func
2) throws an error if a field is not scalar, but has no resolve func
3) throws for any missing field if `resolverValidationOptions.requireResolversForAllFields` = true

@stubailo
Copy link
Contributor

Some feedback:

  1. Perhaps use console.warn instead of .log?
  2. You can test this by overriding console.warn, checking the output, and then overriding it back again.

Thanks for the PR!

@nicolaslopezj
Copy link
Contributor Author

Now we are ready to go.

Is the message ok? Maybe you are using other pattern

Resolve function missing for "${typeName}.${fieldName}". To disable this 
warning check https://github.com/apollostack/graphql-tools/issues/131

@helfer
Copy link
Contributor

helfer commented Sep 10, 2016

Thanks, this looks fine to me. Could you rebase it so I can merge it?

@stubailo stubailo merged commit 82a1c98 into ardatan:master Sep 11, 2016
@nicolaslopezj nicolaslopezj deleted the patch-1 branch September 11, 2016 20:54
@DxCx DxCx mentioned this pull request Sep 13, 2016
7 tasks
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.

4 participants