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

Add schema directives validation #333

Merged

Conversation

javimartinez
Copy link
Contributor

Following the schema validation rules for directives posted in #232, I've implemented the 3 and 4.1 rules in this PR.

I think that due to the type model, it isn't necessary to check the rest of the rules, but I'm no pretty sure, am I missing something?

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you on which rules to check. I just have a comment on directives that are on input values.

val fieldErrorContext: String => String = fieldName => s"Field '$fieldName' of $typeErrorContext"
for {
_ <- validateDirectives[__Type](t, _.directives, typeErrorContext)
_ <- IO.foreach_(t.fields(__DeprecatedArgs(Some(true))).getOrElse(List.empty[__Field]))(f =>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can also be directives in __Type.inputFields and __Field.args: both contain __InputValue which has directives.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen those, added!

}

def validateDirectives[T](
t: T,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You pass T and T => Option[List[Directive]], why not just pass Option[List[Directive]]? Then you won't need the type parameter anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, thank you!

@ghostdogpr ghostdogpr merged commit b219167 into ghostdogpr:master Apr 20, 2020
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.

None yet

2 participants