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 support for descriptions to all AST nodes according to the draft specification #70

Closed

Conversation

almazik
Copy link

@almazik almazik commented Apr 10, 2020

Fixes #33
Description parsing added to the following AST Nodes (matches the current draft specification)::

  • GraphQLSchemaDefinition
  • GraphQLScalarTypeDefinition
  • GraphQLObjectTypeDefinition
  • GraphQLFieldDefinition
  • GraphQLInputValueDefinition
  • GraphQLInterfaceTypeDefinition
  • GraphQLUnionTypeDefinition
  • GraphQLEnumTypeDefinition
  • GraphQLEnumValueDefinition
  • GraphQLInputObjectTypeDefinition
  • GraphQLDirectiveDefinition

This pull request doesn't add the Block Strings descriptions as that should be done on lex parsing level

@sungam3r sungam3r added the enhancement New feature or request label Apr 10, 2020
{
var comment = GetComment();
int start = _currentToken.Start;
int start = description?.Location.Start ?? _currentToken.Start;
Copy link
Member

Choose a reason for hiding this comment

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

GraphQLDescription node has its own location. Why do you set the start of this location to other nodes?

Copy link
Member

Choose a reason for hiding this comment

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

Also I think it would be better to move the GetDescription() inside such ParseXXX methods and remove the parameter. Then the code will be unified with the GetComment() method. How do you think?

Copy link
Author

Choose a reason for hiding this comment

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

GraphQLDescription node has its own location. Why do you set the start of this location to other nodes?

Unlike comments, the description is a part of a formal definition of the parent node. So I followed the approach with other AST nodes. For example, GraphQLObjectTypeDefinition has a name, and so its location is wider than the corresponding GraphQLName's location.

Also I think it would be better to move the GetDescription() inside such ParseXXX methods and remove the parameter. Then the code will be unified with the GetComment() method. How do you think?

I tried to place the description parsing closer to the specific ParseXxx, but there was a problem: when we encounter a string (description), we don't know what kind of node we are parsing yet. So we have to read past the description node in order to find out the name of the entity and branch the logic to the corresponding ParseXxx method. That was the reason why I refactored ParseNamedDefinition into 2 methods: ParseNamedDefinition which parses named nodes without description (and thus immediately knows which node we are at), and ParseNamedDocumentedDefinition which is called when we encounter a string literal (description).

@sungam3r sungam3r added the spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification label Aug 17, 2020
@prowe
Copy link

prowe commented Sep 16, 2020

If this PR has no blocking issues, My team would really appreciate this getting merged..

@almazik
Copy link
Author

almazik commented Sep 16, 2020

We eventually switched to HotChocolate due to blocking issues in graphql-dotnet and no clear progress in resolving them

@sungam3r
Copy link
Member

I am looking forward to reviewing several PRs this month. Too much work, too little free time.

@Shane32
Copy link
Member

Shane32 commented Jul 6, 2021

Superseded by #132

@Shane32 Shane32 closed this Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for descriptions
4 participants