3.13 Directive validation edits #1089
Conversation
- add requirement that DirectiveLocations is non empty - Validation -> TypeValidation - s/directive/Directive in most places
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jbellenger
force-pushed
the
jbellenger-dir-locations
branch
from
68e4c2a
to
cb28e5e
Compare
|
|
||
| 1. A directive definition must not contain the use of a directive which | ||
| 1. A Directive definition must include at least one DirectiveLocation. |
There was a problem hiding this comment.
Why is this worded differently from the other "one or more" constraints?
If this were worded in the same style as Objects, it would read "A Directive must define one or more DirectiveLocations"
To be pedantic, while this sentence would attempt to use a plural form of the DirectiveLocation token, it could also be interpreted as a plural form of the DirectiveLocations token, which is given a distinct definition in Appendix B.4 and does not match the Directive grammar.
I thought that a phrasing that avoided pluralizing DirectiveLocation would be less ambiguous, though recognize there's also value in using consistent language. I'm open to feedback.
There was a problem hiding this comment.
We're also generally okay with must include one or more DirectiveLocation.
benjie
added
the
✏️ Editorial
@jbellenger Please go ahead and make the changes to GraphQL.js 🙌 |
|
Nice! Great to see the spec clarified |
|
GraphQL.js backport into v16 open here: graphql/graphql-js#4137 |
|
Scheduled for discussion in Thursday's WG: https://github.com/graphql/graphql-wg/blob/main/agendas/2024/07-Jul/18-wg-primary.md |
|
cc @graphql/tsc: TL;DR: merging this in one week unless anyone raises concerns. This was discussed at last nights WG (replay; agenda; notes) and it was agreed this is an editorial fix and good to merge. It already has two TSC approvals but I'll leave it open another week or so and then go ahead and merge it. Essentially, we see this as a bugfix. |
The Validation section for Directive types is a little out of step with the validation sections for the other types. In particular, while the grammar definition of Directive requires one or more directive locations, this requirement isn't mentioned in the validation section.
Compare this to the validation sections for other types with "one or more" requirements:
1. An Object type must define one or more fields.1. A Union type must include one or more member types.1. An Enum type must define one or more unique enum values.1. An Input Object type must define one or more input fields.1. An Interface type must define one or more fields.This PR updates Directive validation to be more consistent with the validation sections of other types:
While this PR just clarifies the existing spec, I suspect it might be common for implementations to not check for non-empty directive locations.
I've updated graphql-java and can do the same for graphql-js if I get a tentative OK on this PR.