Refactor SDLPrinter for better indentation #304
Conversation
sungam3r
added
bugfix
github-actions
bot
added
the
test
| @@ -852,7 +856,11 @@ namespace GraphQLParser.Visitors | |||
| public interface IPrintContext : GraphQLParser.Visitors.IASTVisitorContext | |||
| { | |||
| int IndentLevel { get; set; } | |||
| bool IndentPrinted { get; set; } | |||
There was a problem hiding this comment.
@Shane32 Formally, this is breaking changes, but I doubt that there are a sufficient number of people for whom it will be a real problem
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ( | ||
| node is GraphQLInputValueDefinition || | ||
| node is GraphQLFieldDefinition || | ||
| node is GraphQLEnumValueDefinition || | ||
| node is GraphQLRootOperationTypeDefinition || | ||
| node is GraphQLDirectiveLocations && Options.EachDirectiveLocationOnNewLine || | ||
| node is GraphQLUnionMemberTypes && Options.EachUnionMemberOnNewLine | ||
| ) && context.Parents.Count > 0 |
Check notice
Code scanning / CodeQL
Complex condition
| public bool EachDirectiveLocationOnNewLine { get; init; } | ||
| public bool EachUnionMemberOnNewLine { get; init; } | ||
| public int IndentSize { get; init; } |
There was a problem hiding this comment.
To align with other modifiers. Changes from #294 were not published yet.
| @@ -697,15 +764,13 @@ public async Task SelectionSet_Without_Parent_Should_Be_Printed_On_New_Line() | |||
| var printer = new SDLPrinter(new SDLPrinterOptions { PrintComments = true }); | |||
| await printer.PrintAsync(selectionSet, writer); | |||
| writer.ToString().ShouldBe(@"{ | |||
| } | |||
| "); | |||
| }"); | |||
There was a problem hiding this comment.
Better handling for NewLine at the end of printed AST.
| @@ -906,6 +918,7 @@ namespace GraphQLParser.Visitors | |||
| public SDLPrinter() { } | |||
| public SDLPrinter(GraphQLParser.Visitors.SDLPrinterOptions options) { } | |||
| public GraphQLParser.Visitors.SDLPrinterOptions Options { get; } | |||
| protected virtual System.Threading.Tasks.ValueTask MakeVerticalIndentationBetweenTopLevelDefinitions(GraphQLParser.AST.ASTNode node, TContext context) { } | |||
There was a problem hiding this comment.
New method is needed for descendants that may skip nodes, see PrintOnlyStartsWithA. Without it SDLPrinter prints large vertical indentation in place of skipped nodes.
| @@ -575,6 +575,73 @@ type Query | |||
| subscription: S | |||
| } | |||
| """, true, false, false, 5)] | |||
| [InlineData(45, | |||
| """ | |||
| "A component contains the parametric details of a PCB part." | |||
There was a problem hiding this comment.
@nightroman your test now works
| if (value.Length == 0) | ||
| return; | ||
|
|
||
| context.NewLinePrinted = value.Span[value.Length - 1] == '\n'; |
There was a problem hiding this comment.
That approach is much easier than trying to calculate the flag in each place where WriteAsync is called.
| if (CommentedNodeShouldBeCloseToPreviousNode(context, comment, false)) | ||
| await WriteIndentAsync(context).ConfigureAwait(false); | ||
|
|
||
| static bool CommentedNodeShouldBeCloseToPreviousNode(TContext context, GraphQLComment comment, bool start) |
There was a problem hiding this comment.
This was veeery awkward condition.
|
I'm sure there are more issues to fix but for now I finished. I simplified working with indents, especially for comments and descriptions and added more tests to verify edge cases. |
# Conflicts: # src/GraphQLParser.ApiTests/GraphQLParser.approved.txt # src/GraphQLParser/Visitors/StructurePrinter.cs
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #304 +/- ##
==========================================
- Coverage 99.82% 99.77% -0.05%
==========================================
Files 85 85
Lines 4556 4467 -89
Branches 462 448 -14
==========================================
- Hits 4548 4457 -91
- Misses 6 8 +2
Partials 2 2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
So what about version? 8.3 or 9.0? I lean to 8.3 since printer is auxiliary code and not too many people should use custom contexts for printing. |
|
More argument against 9.0 - it will require to bump version of core report and server. |
|
Your call. If it’s source compatible then I’m okay with 8.3. The core repo and server specify 8.2+ probably and don’t need to bump to 9.0 if they don’t rely on any breaking API. |
|
Whatever you think I guess. It really only matters to you if the features are not used by GraphQL. I’d just publish as 9.0 and leave GraphQL 7.x using parser 8.x until we bump GraphQL to 8.0. Again your call. |
|
It is not source compatible in case of custom printer context implemented in user code. I added new members in public interface. |
|
I’m okay with default implementation of new interface members. That reduces breaking changes to .NET Framework users. (Or older obsolete Core users) |
|
I have never used that feature before, will look tomorrow. |
|
Instance state is not allowed for default interfaces so I'll bump to 9. |
fixes #287
waits #303 to be merged into
The core idea of this PR - manage indentation more centrally. Side note - I tried to get rid of trailing NewLine when printing AST.