AstVisitor rework #149

sungam3r · 2021-11-13T00:19:40Z

fixes #11 , supersedes #20

Visitors API was completely rewritten, optimized, tested.

Shane32 · 2021-11-13T00:31:06Z

Is this all synchronous code? Synchronous code that writes streams should be deprecated and replaced with async code, just like what was done with System.Text.Json. Certainly we should not be writing new synchronous code here in 2021. And even more certainly if we are replacing the visitor api, it should be a new async api.

This is a good candidate for using ValueTask, however, to eliminate allocations when writing to a string builder or other non-async stream. Or for any other non-async needs of the visitor api.

sungam3r · 2021-11-13T11:37:04Z

TextWriter has no write methods that return ValueTask.

sungam3r · 2021-11-13T11:50:31Z

Nevermind, I can change all signatures to ValueTask.

sungam3r · 2021-11-13T16:32:09Z

src/Directory.Build.props

@@ -19,7 +19,7 @@
    <!-- https://github.com/clairernovotny/DeterministicBuilds -->
    <ContinuousIntegrationBuild Condition="'$(GITHUB_ACTIONS)' == 'true'">True</ContinuousIntegrationBuild>
    <EnableNETAnalyzers>true</EnableNETAnalyzers>
-    <NoWarn>$(NoWarn);1591</NoWarn> <!--TODO: temporary solution, remove-->
+    <NoWarn>$(NoWarn);1591;IDE1006</NoWarn> <!--TODO: temporary solution, remove-->


Async suffix rule

Just wondering why you decided to have the Visit methods not end in Async?

Year or two ago I would do so, yes. I know about all those recommendations and styles. I control several hundred repos at work unifiing code style/design and this have always been one of the permanent questions. My intuition suggests me that if the API is initially designed as (completely) asynchronous, then it makes no sense to append Async everywhere. It only creates additional noise. By the way, I remove that suppression from dir.props, see .editorconfig file.

sungam3r · 2021-11-13T16:42:28Z

src/GraphQLParser.ApiTests/GraphQL-Parser.approved.txt

@@ -142,7 +143,7 @@ namespace GraphQLParser.AST
        public System.Collections.Generic.List<GraphQLParser.AST.GraphQLDirective>? Directives { get; set; }
        public override GraphQLParser.AST.ASTNodeKind Kind { get; }
        public GraphQLParser.AST.GraphQLSelectionSet? SelectionSet { get; set; }
-        public GraphQLParser.AST.GraphQLNamedType? TypeCondition { get; set; }


see http://spec.graphql.org/October2021/#sec-Inline-Fragments and http://spec.graphql.org/October2021/#FragmentDefinition - TypeCondition node should be added

src/GraphQLParser/AST/GraphQLVariableDefinition.cs

src/GraphQLParser/GraphQLParser.csproj

sungam3r · 2021-11-13T16:53:13Z

src/GraphQLParser/ParserContext.Parse.cs

@@ -1149,6 +1144,7 @@ private GraphQLVariableDefinition ParseVariableDefinition()

            def.Type = ParseType();
            def.DefaultValue = Skip(TokenKind.EQUALS) ? ParseValueLiteral(true) : null;
+            def.Directives = ParseDirectives();


VariableDefinition may have directives

Shane32

This is looking great! Here's a few comments, mostly about escaping strings and adding some cancellation token support.

Shane32 · 2021-11-13T17:00:50Z

src/GraphQLParser/Visitors/SDLWriter.cs

+        {
+            await Visit(stringValue.Comment, context);
+            await context.Writer.WriteAsync('\"');
+            await Write(context, stringValue.Value);


Needs escaping of various characters; please add test.

Optional: detect if the string is multi-line, and can be formatted as a blockstring without loss of data, and if both are true, print it that way.

and can be formatted as a blockstring without loss of data

Example ?

I took test data from lexer tests. This is your test with 30 cases. I changed some values. Honestly, work with escape symbols is very tired. I would like to finish with this PR and go on. I could be mistaken somewhere, further use will show. We will add tests. Or you can add additional test cases yourself where you think right.

I agree to finish the PR and go on. The API will not change if we improve these methods in a later PR, either to fix an issue or enhance performance.

I'm OK to wait 1-2-3 days for your review. At least until I start a new branch working on type extensions. I am 80% sure that I have errors in formatting/edge cases.

src/GraphQLParser/Visitors/SDLWriter.cs

Shane32 · 2021-11-13T17:03:57Z

src/GraphQLParser/Visitors/SDLWriter.cs

+        }
+
+        /// <inheritdoc/>
+        public override async ValueTask VisitDescription(GraphQLDescription description, TContext context)


Not all strings can be written in the blockstring format. For example, a string that starts with a newline. This method should ideally check to be sure the input string can be formatted as a blockstring without loss of information, and if not, be formatted as a string.

and if not, be formatted as a string.

One-line string? Or are you talking about change description node into comment ?

See #149 (comment)

Please show an example here or add a test case yourself. I'm a little confused with all this escaping stuff.

src/GraphQLParser/Visitors/IWriteContext.cs

src/GraphQLParser/Visitors/SDLWriter.cs

src/GraphQLParser/Visitors/StructureWriter.cs

Shane32 · 2021-11-13T17:14:55Z

src/GraphQLParser/Visitors/SDLWriter.cs

+                    case '\r':
+                        break;


Note: with this logic, description strings encoded with CRLF will result in strings being encoded as LF after being decoded. I suggest adding a comment within the method that this change may take place -- or better yet, encode strings containing CRs as strings instead of blockstrings. Tests should be added either way.

strings instead of blockstrings

I'm still confused about string vs blockstring, can you give an example?

By the way I want to rewrite both methods for comment and description node. Now I added the Slice call to get around the problem:

CS4012 Parameters or locals of type 'ReadOnlySpan<char>' cannot be declared in async methods or async lambda expressions.

In addition, I do not really like this char-by-char transmission. At first I wrote a method that finds the chain of non-\r\n chars and writes those spans into writer, but it was rather complicated so I swiched to char-by-char.

src/GraphQLParser/Visitors/SDLWriter.cs

Shane32 · 2021-11-13T17:40:44Z

By the way, should we be adding .ConfigureAwait(false) everywhere?

https://devblogs.microsoft.com/dotnet/configureawait-faq/

sungam3r · 2021-11-13T18:11:25Z

By the way, should we be adding .ConfigureAwait(false) everywhere?

Of course.

add cancellation support add comments

sungam3r · 2021-11-13T21:18:50Z

@Shane32 I have left to solve the issues with strings/blockstrings encoding. I'll do it tomorrow or in the coming days.

sungam3r · 2021-11-13T21:22:33Z

What is done for now:

General design for visitors, naming
Full-async (for netstandard2.1 100%)
Cancellation support
Some bugs were fixed, some new nodes/properties were added

sungam3r · 2021-11-14T17:39:43Z

@Shane32 Waiting for examples.

Shane32

The only major issue I see is that string values are not escaped. You already wrote code within VisitDescription to encode strings; just copy or reuse the code block and it should be fine. I can do a detailed review of the encoding for strings and blockstrings at a later time and add another PR if I see any other suggestions. Ok?

src/GraphQLParser/Visitors/SDLWriter.cs

sungam3r · 2021-11-19T21:03:03Z

ping @Shane32 , any suggestions/comments/test cases?

Shane32 · 2021-11-19T21:15:42Z

Not at this time.

codecov-commenter · 2022-01-02T14:09:01Z

Codecov Report

Merging #149 (d310867) into master (ebb76f3) will decrease coverage by 2.30%.
The diff coverage is 81.21%.

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   91.91%   89.61%   -2.31%     
==========================================
  Files          50       55       +5     
  Lines        2498     3263     +765     
  Branches      278      333      +55     
==========================================
+ Hits         2296     2924     +628     
- Misses        180      300     +120     
- Partials       22       39      +17

Impacted Files	Coverage Δ
src/GraphQLParser/AST/GraphQLArgument.cs	`72.72% <ø> (+9.09%)`	⬆️
src/GraphQLParser/AST/GraphQLComment.cs	`50.00% <ø> (ø)`
src/GraphQLParser/AST/GraphQLDescription.cs	`50.00% <ø> (ø)`
src/GraphQLParser/AST/GraphQLFragmentDefinition.cs	`70.00% <ø> (+10.00%)`	⬆️
src/GraphQLParser/AST/GraphQLListType.cs	`63.63% <ø> (ø)`
src/GraphQLParser/AST/GraphQLListValue.cs	`66.66% <0.00%> (-17.34%)`	⬇️
src/GraphQLParser/AST/GraphQLName.cs	`100.00% <ø> (+20.00%)`	⬆️
src/GraphQLParser/AST/GraphQLNamedType.cs	`63.63% <ø> (-9.10%)`	⬇️
src/GraphQLParser/AST/GraphQLNonNullType.cs	`63.63% <ø> (ø)`
src/GraphQLParser/AST/GraphQLObjectField.cs	`63.63% <ø> (+9.09%)`	⬆️
... and 40 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 ebb76f3...d310867. Read the comment docs.

AstVisitor rework

63ba1f8

sungam3r requested a review from Shane32 November 13, 2021 00:19

github-actions bot added the test Pull request that adds new or changes existing tests label Nov 13, 2021

sungam3r mentioned this pull request Nov 13, 2021

AST printer #20

Closed

async visitor API + TypeCondition node

877b608

sungam3r added BREAKING Breaking changes in either public API or runtime behavior enhancement New feature or request spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification labels Nov 13, 2021

sungam3r commented Nov 13, 2021

View reviewed changes

oops

5f0bb90

github-actions bot added the code style Pull request that applies code style rules label Nov 13, 2021

sungam3r commented Nov 13, 2021

View reviewed changes

src/GraphQLParser/AST/GraphQLVariableDefinition.cs Show resolved Hide resolved

sungam3r commented Nov 13, 2021

View reviewed changes

src/GraphQLParser/GraphQLParser.csproj Show resolved Hide resolved

sungam3r commented Nov 13, 2021

View reviewed changes

sungam3r added 3 commits November 13, 2021 20:01

rename

719ef48

format

813adb2

oops

2dfa4e4

Shane32 requested changes Nov 13, 2021

View reviewed changes

sungam3r added 6 commits November 13, 2021 23:02

remove kind field from GraphQLListValue

01240a5

add cancellation support add comments

reorder ASTNodes properties according to the spec grammar

6cf5c77

encoding

405f6ac

configureawait everywhere

856d2f0

format

4d78fa5

Parent -> Parents

c0b47af

sungam3r added 3 commits November 14, 2021 01:25

add GraphQLAlias node

de2900a

support for escape symbols

fa22a0b

format

0507a9c

sungam3r added 2 commits November 14, 2021 20:41

format

d562bc8

Add documentation + options for StructureWriter

ea317de

github-actions bot added the documentation An issue or pull request regarding documentation improvements label Nov 14, 2021

Shane32 requested changes Nov 14, 2021

View reviewed changes

src/GraphQLParser/Visitors/SDLWriter.cs Outdated Show resolved Hide resolved

encode strings

18dcf68

Shane32 approved these changes Nov 19, 2021

View reviewed changes

sungam3r added 2 commits December 29, 2021 01:46

Merge branch 'master' into visitor

150cb37

Merge branch 'master' into visitor

d310867

sungam3r added this to the 8.0 milestone Jan 2, 2022

sungam3r self-assigned this Jan 2, 2022

sungam3r merged commit 4a55a62 into master Jan 2, 2022

sungam3r deleted the visitor branch January 2, 2022 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AstVisitor rework #149

AstVisitor rework #149

sungam3r commented Nov 13, 2021

Shane32 commented Nov 13, 2021

sungam3r commented Nov 13, 2021

sungam3r commented Nov 13, 2021

sungam3r Nov 13, 2021

Shane32 Nov 14, 2021

sungam3r Nov 16, 2021

sungam3r Nov 13, 2021

sungam3r Nov 13, 2021

Shane32 left a comment

Shane32 Nov 13, 2021

sungam3r Nov 13, 2021

sungam3r Nov 14, 2021

Shane32 Nov 14, 2021

sungam3r Nov 16, 2021

Shane32 Nov 13, 2021

sungam3r Nov 13, 2021

sungam3r Nov 14, 2021

Shane32 Nov 13, 2021

sungam3r Nov 13, 2021

Shane32 commented Nov 13, 2021

sungam3r commented Nov 13, 2021

sungam3r commented Nov 13, 2021

sungam3r commented Nov 13, 2021

sungam3r commented Nov 14, 2021

Shane32 left a comment

sungam3r commented Nov 19, 2021

Shane32 commented Nov 19, 2021

codecov-commenter commented Jan 2, 2022

AstVisitor rework #149

AstVisitor rework #149

Conversation

sungam3r commented Nov 13, 2021

Shane32 commented Nov 13, 2021

sungam3r commented Nov 13, 2021

sungam3r commented Nov 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shane32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shane32 commented Nov 13, 2021

sungam3r commented Nov 13, 2021

sungam3r commented Nov 13, 2021

sungam3r commented Nov 13, 2021

sungam3r commented Nov 14, 2021

Shane32 left a comment

Choose a reason for hiding this comment

sungam3r commented Nov 19, 2021

Shane32 commented Nov 19, 2021

codecov-commenter commented Jan 2, 2022

Codecov Report