Skip to content

Fix inconsistent formatting of arguments #358

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

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Fix inconsistent formatting of arguments #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/GraphQLParser.ApiTests/GraphQLParser.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) { }
Expand All @@ -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; }
Expand Down
111 changes: 94 additions & 17 deletions src/GraphQLParser.Tests/Visitors/SDLPrinterFromParsedTextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -606,7 +617,7 @@ input DesComponentFilterInput {
revision: DesRevisionFilterInput
}
""")]
[InlineData(46,
[InlineData(48,
"""
# comment
directive @my on FIELD
Expand All @@ -615,7 +626,7 @@ directive @my on FIELD
# comment
directive @my on FIELD
""")]
[InlineData(47,
[InlineData(49,
"""
query q
# comment
Expand All @@ -628,7 +639,7 @@ query q
x
}
""")]
[InlineData(48,
[InlineData(50,
"""
query q
(
Expand All @@ -642,7 +653,7 @@ query q(
x
}
""")]
[InlineData(49,
[InlineData(51,
"""
"description"
schema {
Expand All @@ -655,7 +666,7 @@ query q(
query: Query
}
""")]
[InlineData(50,
[InlineData(52,
"""
type Query {
"Fetches an object given its ID."
Expand Down Expand Up @@ -691,7 +702,7 @@ type Query {
id: ID!): DesWorkspace
}
""")]
[InlineData(51,
[InlineData(53,
"""
type Query {
user
Expand All @@ -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
Expand All @@ -726,7 +779,7 @@ directive @my
# comment 2
arg: Boolean!) on FIELD
""")]
[InlineData(53,
[InlineData(57,
"""
query Q {
field1(arg1: 1) {
Expand All @@ -745,7 +798,7 @@ query Q {
}
}
""")]
[InlineData(54,
[InlineData(58,
"""
query Q {
field1
Expand Down Expand Up @@ -791,7 +844,7 @@ query Q {
}
}
""")]
[InlineData(55,
[InlineData(59,
"""
fragment f
#comment
Expand All @@ -804,7 +857,7 @@ on Person {
name
}
""")]
[InlineData(56,
[InlineData(60,
"""
type Person
#comment
Expand All @@ -817,7 +870,7 @@ implements Entity {
name: String
}
""")]
[InlineData(57,
[InlineData(61,
"""
type Person
#comment
Expand All @@ -834,7 +887,7 @@ implements Entity &
name: String
}
""")]
[InlineData(58,
[InlineData(62,
""""
"description"
type Person {
Expand All @@ -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,
Expand All @@ -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
{
Expand All @@ -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();
Expand Down
3 changes: 3 additions & 0 deletions src/GraphQLParser/AST/Definitions/GraphQLInputValueDefinition.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
Expand Down
57 changes: 45 additions & 12 deletions src/GraphQLParser/Visitors/SDLPrinter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand All @@ -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>
Expand Down
Loading