Add support for description parsing #132
Conversation
github-actions
bot
added
the
test
Shane32
added
enhancement
github-actions
bot
added
the
test
Codecov Report
@@ Coverage Diff @@
## master #132 +/- ##
==========================================
+ Coverage 92.06% 92.58% +0.52%
==========================================
Files 48 49 +1
Lines 2193 2320 +127
Branches 251 273 +22
==========================================
+ Hits 2019 2148 +129
+ Misses 147 145 -2
Partials 27 27
Continue to review full report at Codecov.
|
|
I have incorporated ideas from #70 into this PR. If there are no comments I will merge this PR. |
|
@Shane32 I begin to review this PR but I already suspect what it will affect: |
|
I can't remember exactly, but I think I made changes that would be both non-breaking (i.e. support comments as descriptions) and also support the spec (i.e. support strings/blockstrings as descriptions). Precedence is to spec - a string/blockstring is a description. If no string/blockstring, then resort to old non-spec behavior. Per spec, I believe comments should be completely ignored. Perhaps we change that in v5. |
| @@ -0,0 +1,9 @@ | |||
| namespace GraphQLParser.AST | |||
| { | |||
| public class GraphQLDescription : ASTNode | |||
There was a problem hiding this comment.
@Shane32 Where is Location for GraphQLDescription syntax node ?
There was a problem hiding this comment.
I think I didn't put one in because the syntax for the parent node technically includes the description, and so the start of the description is the same as the start of the type definition node. At least that's maybe why I did it??? It's been 4 months so I can't remember my reasoning exactly.
Maybe it should be in there anyway though, so that there's a way to determine the end of the description node? I guess that would be a new class called GraphQLDescriptionFull ... ??
There was a problem hiding this comment.
the syntax for the parent node technically includes the description, and so the start of the description is the same as the start of the type definition node.
Nope. Those are different syntax nodes with different locations. GraphQLComment node has Location for example.
I guess that would be a new class called
GraphQLDescriptionFull
Yep. Maybe new, maybe existing. I should remember why I did only one GraphQLComment class without GraphQLCommentFull counterpart. I have already started review, so I can do all the necessary edits myself.
There was a problem hiding this comment.
Pretty sure the library is already written this way -- to match the spec. So a type definition node will have its Location start and end points exactly as described by the spec and shown above.
I don't see any reason why a description node can't have a Location also
There was a problem hiding this comment.
There is no added complexity. It is not complex. It is not difficult to write. In fact you simply don’t need to remove optimized functionality that already exists in the library. I do not see any reason to take something optimized and unoptimize it. It does not make sense.
There was a problem hiding this comment.
The current implementation adds a couple lines of code to the entire library. As in like simply 2-3 lines of code. That is not complex. It is not harder to read. There is no reason to deoptimize the library for the sake of 2 lines of code.
There was a problem hiding this comment.
I will think. There is a small chance that schema extensions can also have descriptions (although it is unlikely) - graphql/graphql-spec#900
There was a problem hiding this comment.
Well, everything turned out to be simple and difficult at the same time. See http://spec.graphql.org/October2021/#sec-Type-Extensions or even more old http://spec.graphql.org/June2018/#sec-Type-Extensions :
Now parser supports (partially) only ObjectTypeExtension. Wow! :) No support for other extensions at all, no tests. The good news is that Description property will not exist on GraphQLTypeExtensionDefinition when it stops inherit from GraphQLTypeDefinition. All extensions should have a separate new base class. Also we should add a bunch of new classes to cover each kind of extension. Looks like a good feature for v8. So I suggest to merge this as is (after your review of course) and continue to work on separate issue - #143.
|
Yep, I see |
Adds support for description parsing according to spec. Requires #131 to be merged in order for tests to pass.
Note in the case where there is a comment both before and after the description, such as in the example below, the second comment is attached to the type definition, and the first comment is added to the list of unattached comments. If a comment only in one of the two locations, then it is attached to the type definition. This mechanic is covered by the tests.
Description fields have been added only to the nodes that allow a description. Since typical GraphQL queries and mutations cannot contain any nodes that would have a description field, and therefor no additional memory could be allocated, there is no need for an additional option to skip parsing of descriptions.