Fix inconsistent formatting of arguments (#358)

Author: sungam3r

src/GraphQLParser.ApiTests/GraphQLParser.approved.txt

@@ -907,6 +907,12 @@ namespace GraphQLParser.Visitors
             public System.IO.TextWriter Writer { get; }
         }
     }
+    public enum SDLPrinterArgumentsMode
+    {
+        None = 0,
+        PreferNewLine = 1,
+        ForceNewLine = 2,
+    }
     public static class SDLPrinterExtensions
     {
         public static string Print(this GraphQLParser.Visitors.SDLPrinter printer, GraphQLParser.AST.ASTNode node) { }
@@ -916,6 +922,7 @@ namespace GraphQLParser.Visitors
     public class SDLPrinterOptions
     {
         public SDLPrinterOptions() { }
+        public GraphQLParser.Visitors.SDLPrinterArgumentsMode ArgumentsPrintMode { get; set; }
         public bool EachDirectiveLocationOnNewLine { get; set; }
         public bool EachUnionMemberOnNewLine { get; set; }
         public int IndentSize { get; set; }

src/GraphQLParser.Tests/Visitors/SDLPrinterFromParsedTextTests.cs

@@ -557,16 +557,27 @@ type Query {
 @"directive @skip(""Skipped when true."" if: Boolean!, x: Some) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT",
 @"directive @skip(
   ""Skipped when true.""
-  if: Boolean!, x: Some) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT")]
+  if: Boolean!, x: Some) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT", true, true, false, false, 2, SDLPrinterArgumentsMode.None)]
     [InlineData(43,
+@"directive @skip(if: Boolean!, x: Some) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT",
+@"directive @skip(
+  if: Boolean!,
+  x: Some) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT", true, true, false, false, 2, SDLPrinterArgumentsMode.ForceNewLine)]
+    [InlineData(44,
+@"directive @skip(""Skipped when true."" if: Boolean!, x: Some) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT",
+@"directive @skip(
+  ""Skipped when true.""
+  if: Boolean!,
+  x: Some) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT")]
+    [InlineData(45,
 """directive @skip("Skipped when true." if: Boolean!, "Second argument" x: Some) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT""", """
 directive @skip(
   "Skipped when true."
   if: Boolean!,
   "Second argument"
   x: Some) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
 """)]
-    [InlineData(44,
+    [InlineData(46,
 "schema { query: Q mutation: M subscription: S }",
 """
 schema {
@@ -575,7 +586,7 @@ directive @skip(
      subscription: S
 }
 """, true, true, false, false, 5)]
-    [InlineData(45,
+    [InlineData(47,
 """
 "A component contains the parametric details of a PCB part."
 input DesComponentFilterInput {
@@ -606,7 +617,7 @@ input DesComponentFilterInput {
   revision: DesRevisionFilterInput
 }
 """)]
-    [InlineData(46,
+    [InlineData(48,
 """
 # comment
 directive @my on FIELD
@@ -615,7 +626,7 @@ directive @my on FIELD
 # comment
 directive @my on FIELD
 """)]
-    [InlineData(47,
+    [InlineData(49,
 """
 query q
 # comment
@@ -628,7 +639,7 @@ query q
   x
 }
 """)]
-    [InlineData(48,
+    [InlineData(50,
 """
 query q
 (
@@ -642,7 +653,7 @@ query q(
   x
 }
 """)]
-    [InlineData(49,
+    [InlineData(51,
 """
 "description"
 schema {
@@ -655,7 +666,7 @@ query q(
   query: Query
 }
 """)]
-    [InlineData(50,
+    [InlineData(52,
 """
 type Query {
 "Fetches an object given its ID."
@@ -691,7 +702,7 @@ type Query {
     id: ID!): DesWorkspace
 }
 """)]
-    [InlineData(51,
+    [InlineData(53,
 """
 type Query {
   user
@@ -710,8 +721,50 @@ type Query {
     # comment 2
     id: ID!, name: Name!): Node
 }
+""", true, true, false, false, 2, SDLPrinterArgumentsMode.None)]
+    [InlineData(54,
+"""
+type Query {
+  user
+  # comment 1
+  (
+    # comment 2
+    id: ID!
+    name: Name!): Node
+}
+""",
+"""
+type Query {
+  user
+  # comment 1
+  (
+    # comment 2
+    id: ID!,
+    name: Name!): Node
+}
+""", true, true, false, false, 2, SDLPrinterArgumentsMode.ForceNewLine)]
+    [InlineData(55,
+"""
+type Query {
+  user
+  # comment 1
+  (
+    # comment 2
+    id: ID!
+    name: Name!): Node
+}
+""",
+"""
+type Query {
+  user
+  # comment 1
+  (
+    # comment 2
+    id: ID!,
+    name: Name!): Node
+}
 """)]
-    [InlineData(52,
+    [InlineData(56,
 """
 directive @my
   # comment 1
@@ -726,7 +779,7 @@ directive @my
     # comment 2
     arg: Boolean!) on FIELD
 """)]
-    [InlineData(53,
+    [InlineData(57,
 """
 query Q {
   field1(arg1: 1) {
@@ -745,7 +798,7 @@ query Q {
   }
 }
 """)]
-    [InlineData(54,
+    [InlineData(58,
 """
 query Q {
   field1
@@ -791,7 +844,7 @@ query Q {
   }
 }
 """)]
-    [InlineData(55,
+    [InlineData(59,
 """
 fragment f
 #comment
@@ -804,7 +857,7 @@ on Person {
   name
 }
 """)]
-    [InlineData(56,
+    [InlineData(60,
 """
 type Person
 #comment
@@ -817,7 +870,7 @@ implements Entity {
   name: String
 }
 """)]
-    [InlineData(57,
+    [InlineData(61,
 """
 type Person
 #comment
@@ -834,7 +887,7 @@ implements Entity &
   name: String
 }
 """)]
-    [InlineData(58,
+    [InlineData(62,
 """"
 "description"
 type Person {
@@ -846,6 +899,28 @@ type Person {
   name: String
 }
 """, false, false)]
+    [InlineData(63, // https://github.com/graphql-dotnet/parser/issues/330
+""""
+type DesPcb {
+  designItems("An optional array of designators to search." designators: [String!] "Returns the first _n_ elements from the list." first: Int "Returns the elements in the list that come after the specified cursor." after: String "Returns the last _n_ elements from the list." last: Int "Returns the elements in the list that come before the specified cursor." before: String where: DesDesignItemFilterInput): DesDesignItemConnection
+}
+"""",
+"""
+type DesPcb {
+  designItems(
+    "An optional array of designators to search."
+    designators: [String!],
+    "Returns the first _n_ elements from the list."
+    first: Int,
+    "Returns the elements in the list that come after the specified cursor."
+    after: String,
+    "Returns the last _n_ elements from the list."
+    last: Int,
+    "Returns the elements in the list that come before the specified cursor."
+    before: String,
+    where: DesDesignItemFilterInput): DesDesignItemConnection
+}
+""")]
     public async Task SDLPrinter_Should_Print_Document(
         int number,
         string text,
@@ -854,7 +929,8 @@ public async Task SDLPrinter_Should_Print_Document(
         bool writeDescriptions = true,
         bool eachDirectiveLocationOnNewLine = false,
         bool eachUnionMemberOnNewLine = false,
-        int indentSize = 2)
+        int indentSize = 2,
+        SDLPrinterArgumentsMode mode = SDLPrinterArgumentsMode.PreferNewLine)
     {
         var printer = new SDLPrinter(new SDLPrinterOptions
         {
@@ -863,6 +939,7 @@ public async Task SDLPrinter_Should_Print_Document(
             EachDirectiveLocationOnNewLine = eachDirectiveLocationOnNewLine,
             EachUnionMemberOnNewLine = eachUnionMemberOnNewLine,
             IndentSize = indentSize,
+            ArgumentsPrintMode = mode,
         });
         var writer = new StringWriter();
         var document = text.Parse();

src/GraphQLParser/AST/Definitions/GraphQLInputValueDefinition.cs

@@ -1,8 +1,11 @@
+using System.Diagnostics;
+
 namespace GraphQLParser.AST;
 
 /// <summary>
 /// AST node for <see cref="ASTNodeKind.InputValueDefinition"/>.
 /// </summary>
+[DebuggerDisplay("GraphQLInputValueDefinition: {Name}: {Type}")]
 public class GraphQLInputValueDefinition : GraphQLTypeDefinition, IHasDirectivesNode, IHasDefaultValueNode
 {
     internal GraphQLInputValueDefinition()

src/GraphQLParser/Visitors/SDLPrinter.cs

@@ -480,13 +480,6 @@ protected override async ValueTask VisitInputObjectTypeExtensionAsync(GraphQLInp
     /// <inheritdoc/>
     protected override async ValueTask VisitInputValueDefinitionAsync(GraphQLInputValueDefinition inputValueDefinition, TContext context)
     {
-        bool hasParent = TryPeekParent(context, out var parent);
-
-        if (hasParent && parent is GraphQLArgumentsDefinition argsDef && argsDef.Items.IndexOf(inputValueDefinition) > 0)
-        {
-            await context.WriteAsync(inputValueDefinition.Description == null ? ", " : ",").ConfigureAwait(false);
-        }
-
         await VisitAsync(inputValueDefinition.Comments, context).ConfigureAwait(false);
         await VisitAsync(inputValueDefinition.Description, context).ConfigureAwait(false);
         await VisitAsync(inputValueDefinition.Name, context).ConfigureAwait(false);
@@ -714,10 +707,12 @@ protected override async ValueTask VisitArgumentsDefinitionAsync(GraphQLArgument
     {
         await VisitAsync(argumentsDefinition.Comments, context).ConfigureAwait(false);
         await VisitAsync(LiteralNode.Wrap("("), context).ConfigureAwait(false);
-
-        foreach (var argumentDefinition in argumentsDefinition.Items)
-            await VisitAsync(argumentDefinition, context).ConfigureAwait(false);
-
+        for (int i = 0; i < argumentsDefinition.Items.Count; ++i)
+        {
+            await VisitAsync(argumentsDefinition.Items[i], context).ConfigureAwait(false);
+            if (i < argumentsDefinition.Items.Count - 1)
+                await context.WriteAsync(",").ConfigureAwait(false);
+        }
         await context.WriteAsync(")").ConfigureAwait(false);
     }
 
@@ -905,9 +900,39 @@ node is GraphQLInputValueDefinition
         if ((context.LastVisitedNode is GraphQLFragmentSpread || context.LastVisitedNode is GraphQLSelectionSet) && !context.IndentPrinted)
             await context.WriteLineAsync().ConfigureAwait(false);
 
+        // ensure NewLine before printing argument if previous node did not print NewLine
+        // https://github.com/graphql-dotnet/parser/issues/330
+        _ = TryPeekParent(context, out var parent);
+        if (!context.IndentPrinted && node is GraphQLInputValueDefinition && parent is GraphQLArgumentsDefinition arguments)
+        {
+            switch (Options.ArgumentsPrintMode)
+            {
+                case SDLPrinterArgumentsMode.ForceNewLine:
+                    await context.WriteLineAsync().ConfigureAwait(false);
+                    break;
+
+                case SDLPrinterArgumentsMode.PreferNewLine:
+                    foreach (var arg in arguments.Items)
+                    {
+                        if (HasPrintableComments(arg) || HasPrintableDescription(arg))
+                        {
+                            await context.WriteLineAsync().ConfigureAwait(false);
+                            break;
+                        }
+                    }
+                    break;
+
+                default:
+                    break;
+            }
+        }
+
         // ensure proper indentation on the current line before printing new node
         if (context.NewLinePrinted)
             await WriteIndentAsync(context).ConfigureAwait(false);
+        // otherwise ensure single whitespace indentation for all arguments in list except the first one
+        else if (parent is GraphQLArgumentsDefinition argsDef && node is GraphQLInputValueDefinition input && argsDef.Items.IndexOf(input) != 0)
+            await context.WriteAsync(" ").ConfigureAwait(false);
 
         if (node is LiteralNode literalNode) // base.VisitAsync will throw on unknown node
             await context.WriteAsync(literalNode.Literal).ConfigureAwait(false);
@@ -969,6 +994,8 @@ private async ValueTask WriteIndentAsync(TContext context)
 
     private bool HasPrintableComments(ASTNode node) => node.Comments?.Count > 0 && Options.PrintComments;
 
+    private bool HasPrintableDescription(IHasDescriptionNode node) => node.Description != null && Options.PrintDescriptions;
+
     // Returns parent if called inside VisitXXX i.e. after context.Parents.Push(node);
     // Returns grand-parent if called inside VisitAsync i.e. before context.Parents.Push(node);
     private static bool TryPeekParent(TContext context, [NotNullWhen(true)] out ASTNode? node)
@@ -1136,6 +1163,12 @@ public class SDLPrinterOptions
     /// </summary>
     public bool EachUnionMemberOnNewLine { get; set; }
 
+    /// <summary>
+    /// How to print each argument definition.
+    /// By default <see cref="SDLPrinterArgumentsMode.PreferNewLine"/>.
+    /// </summary>
+    public SDLPrinterArgumentsMode ArgumentsPrintMode { get; set; } = SDLPrinterArgumentsMode.PreferNewLine;
+
     /// <summary>
     /// The size of the horizontal indentation in spaces.
     /// By default 2.
@@ -1144,7 +1177,7 @@ public class SDLPrinterOptions
 }
 
 /// <summary>
-/// Preudo AST node to allow calls to <see cref="SDLPrinter{TContext}.VisitAsync(ASTNode?, TContext)"/>
+/// Pseudo AST node to allow calls to <see cref="SDLPrinter{TContext}.VisitAsync(ASTNode?, TContext)"/>
 /// for indentation purposes. Any literal printed first after optional comment or description nodes in
 /// any VisitXXX method should be wrapped into <see cref="LiteralNode"/> for proper indentation.
 /// </summary>