feat(compiler, parser): support executable document descriptions#974
feat(compiler, parser): support executable document descriptions#974goto-bus-stop wants to merge 2 commits intomainfrom
Conversation
Support for parsing and serializing descriptions on: - (longhand) operation definitions - fragment definitions - variable definitions As described by the latest GraphQL spec draft: graphql/graphql-spec#1170 Adding a description on a query shorthand (bare braced selection set) raises a parse error. This is a breaking change for apollo-compiler due to the addition of new fields: - `apollo_compiler::ast::OperationDefinition::description` - `apollo_compiler::ast::FragmentDefinition::description` - `apollo_compiler::ast::VariableDefinition::description` - `apollo_compiler::executable::Operation::description` - `apollo_compiler::executable::Fragment::description` For most users, adding `description: None` anywhere they create one of these structures is good enough.
lrlna
left a comment
lrlna
left a comment
There was a problem hiding this comment.
This looks good!
The only thing that needs to change is that the new file in apollo-compiler/test_data/diagnostics is missing it's matching .txt pair. Other comments are clarification questions.
Since this is a breaking change for the compiler, do we need to have a conversation about what should go into the next major version of the compiler (if there is anything)?
...es/apollo-compiler/test_data/serializer/diagnostics/0125_query_shorthand_description.graphql
Show resolved
Hide resolved
There was a problem hiding this comment.
is this missing a matching .txt file?
There was a problem hiding this comment.
also, should we have this test case in the parser's test_data directory for posterity?
There was a problem hiding this comment.
should we also have this test case in the parser's test_data directory for posterity?
I reviewed the PR before seeing your message in slack asking the exact same thing. Carry on! |
| @@ -33,6 +41,11 @@ pub(crate) fn variable_definitions(p: &mut Parser) { | |||
| /// Variable **:** Type DefaultValue? Directives[Const]? | |||
There was a problem hiding this comment.
The comments on the parse functions that were modified should be updated to reflect the new grammar (add Description?)
| impl From<FragmentDef> for ast::Definition { | ||
| fn from(x: FragmentDef) -> Self { | ||
| ast::FragmentDefinition { | ||
| description: None, // TODO(@goto-bus-stop): represent description |
There was a problem hiding this comment.
There's a few TODOs here in the PR where we should just propagate the description from the input value
3 participants
Support for parsing and serializing descriptions on:
As described by the latest GraphQL spec draft: graphql/graphql-spec#1170
Adding a description on a query shorthand (bare braced selection set) raises a parse error.
This is a breaking change for apollo-compiler due to the addition of new fields:
apollo_compiler::ast::OperationDefinition::descriptionapollo_compiler::ast::FragmentDefinition::descriptionapollo_compiler::ast::VariableDefinition::descriptionapollo_compiler::executable::Operation::descriptionapollo_compiler::executable::Fragment::descriptionFor most users, adding
description: Noneanywhere they create one of these structures is good enough.Todos