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.

Sign up for GitHub

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

Jump to bottom

Add support for type extensions #154

Merged
merged 1 commit into from
Jan 2, 2022
Merged

Add support for type extensions #154

merged 1 commit into from
Jan 2, 2022

Conversation

sungam3r
Copy link
Member

@sungam3r sungam3r commented Jan 2, 2022

fixes #143

@sungam3r sungam3r requested a review from Shane32 January 2, 2022 18:52
@sungam3r sungam3r self-assigned this Jan 2, 2022
@github-actions github-actions bot added documentation An issue or pull request regarding documentation improvements test Pull request that adds new or changes existing tests labels Jan 2, 2022
sungam3r
sungam3r commented Jan 2, 2022
View reviewed changes
src/GraphQLParser.Tests/ParserTests.cs
@@ -773,12 +773,63 @@ public void Should_Parse_Unions(string text)
}

[Theory]
[InlineData("type Query", ASTNodeKind.ObjectTypeDefinition)]
[InlineData("extend type Query", ASTNodeKind.TypeExtensionDefinition)]
Copy link
Member Author

Choose a reason for hiding this comment

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

extend type Query actually is invalid query :) , see Should_Throw_Extensions

sungam3r
sungam3r commented Jan 2, 2022
View reviewed changes
src/GraphQLParser.Tests/ParserTests.cs
[InlineData("input Empty", ASTNodeKind.InputObjectTypeDefinition)]
[InlineData("interface Empty", ASTNodeKind.InterfaceTypeDefinition)]
[InlineData("enum Empty", ASTNodeKind.EnumTypeDefinition)]
[InlineData("extend type Type implements Interface", ASTNodeKind.TypeExtensionDefinition)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved into Should_Parse_Extensions

@codecov-commenter
Copy link

Codecov Report

Merging #154 (d9e16bc) into master (4a55a62) will decrease coverage by 3.04%.
The diff coverage is 65.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
- Coverage   89.61%   86.56%   -3.05%     
==========================================
  Files          55       61       +6     
  Lines        3263     3559     +296     
  Branches      333      363      +30     
==========================================
+ Hits         2924     3081     +157     
- Misses        300      430     +130     
- Partials       39       48       +9
Impacted Files Coverage Δ
...c/GraphQLParser/AST/GraphQLObjectTypeDefinition.cs 83.33% <ø> (ø)
src/GraphQLParser/Visitors/DefaultNodeVisitor.cs 49.34% <13.95%> (-6.01%) ⬇️
src/GraphQLParser/Visitors/SDLWriter.cs 82.72% <44.64%> (-7.03%) ⬇️
src/GraphQLParser/AST/GraphQLEnumTypeExtension.cs 54.54% <54.54%> (ø)
...aphQLParser/AST/GraphQLInputObjectTypeExtension.cs 54.54% <54.54%> (ø)
src/GraphQLParser/AST/GraphQLUnionTypeExtension.cs 54.54% <54.54%> (ø)
...GraphQLParser/AST/GraphQLInterfaceTypeExtension.cs 58.33% <58.33%> (ø)
src/GraphQLParser/NodeHelper.cs 93.76% <59.18%> (-5.31%) ⬇️
...rc/GraphQLParser/AST/GraphQLObjectTypeExtension.cs 75.00% <75.00%> (ø)
src/GraphQLParser/ParserContext.Parse.cs 98.49% <99.35%> (+0.32%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a55a62...d9e16bc. Read the comment docs.

@sungam3r sungam3r mentioned this pull request Jan 2, 2022
Merged
@sungam3r sungam3r added enhancement New feature or request spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification BREAKING Breaking changes in either public API or runtime behavior labels Jan 2, 2022
sungam3r
sungam3r commented Jan 2, 2022
View reviewed changes
src/GraphQLParser/ParserContext.Parse.cs
var name = ParseName();
var args = ParseArgumentDefs();
bool repeatable = ParseRepeatable();
var def = NodeHelper.CreateGraphQLDirectiveDefinition(_ignoreOptions);
Copy link
Member Author

Choose a reason for hiding this comment

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

I slightly changed all the ParseXXX methods:

  private GraphQLScalarValue ParseStringValue(/*bool isConstant*/)
        {
            var token = _currentToken;  // store token before node creation
                                                         // new line
            var val = NodeHelper.CreateGraphQLScalarValue(_ignoreOptions, ASTNodeKind.StringValue); // create node
                                                         // new line
            val.Comment = GetComment();    // store comment first
            Advance();                                     // some setup here
            val.Value = token.Value;
            val.Location = GetLocation(token.Start);
                                                                 // new line before return
            return val;
        }

I moved the initialization of node properties to a later stage (after node creation) so that it matches the grammar specification as much as possible (only in terms of reading).

sungam3r
sungam3r commented Jan 2, 2022
View reviewed changes
src/GraphQLParser/ParserContext.Parse.cs
ExpectKeyword("union");
def.Name = ParseName();
def.Directives = ParseDirectives();
def.Types = Peek(TokenKind.EQUALS) ? ParseUnionMemberTypes() : null;
Copy link
Member Author

Choose a reason for hiding this comment

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

Bugfix here. Union definition may be empty.
изображение

sungam3r
sungam3r commented Jan 2, 2022
View reviewed changes
src/GraphQLParser/Visitors/SDLWriter.cs

if (interfaceTypeDefinition.Fields?.Count > 0)
{
await context.WriteLine().ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Three lines (and two more below) were moved under if to handle empty interfaces.

@sungam3r
Copy link
Member Author

sungam3r commented Jan 2, 2022

OK to merge? No comments?

@Shane32
Copy link
Member

Shane32 commented Jan 2, 2022

Yes. No comments; whatever you think.

@sungam3r sungam3r merged commit d9e12e4 into master Jan 2, 2022
@sungam3r sungam3r deleted the extensions branch January 2, 2022 23:25
@sungam3r sungam3r added this to the 8.0 milestone Jan 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shane32 Shane32 Shane32 approved these changes

Assignees

@sungam3r sungam3r

Labels
BREAKING Breaking changes in either public API or runtime behavior documentation An issue or pull request regarding documentation improvements enhancement New feature or request spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification test Pull request that adds new or changes existing tests
Projects
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.

Add support for all types of schema/type extensions
3 participants
@sungam3r @codecov-commenter @Shane32