graphql-dotnet / parser

A lexer and parser for GraphQL in .NET
MIT License
216 stars 43 forks source link

Inconsistent formatting of arguments #330

Closed nightroman closed 1 year ago

nightroman commented 1 year ago

Please find attached the project for reproducing the issue and follow the below steps. TryGraphQLParser.zip

Steps

x1-type.graphql is the input file to be formatted

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
}

Run

dotnet run -- x1-type.graphql

This produces the new x1-type.graphql.output.graphql. The field arguments are formatted mostly as expected:

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
}

But the last argument where: DesDesignItemFilterInput formatting is inconsistent, it is expected to be on its own line:

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
}
Shane32 commented 1 year ago

@sungam3r can you look at this?

sungam3r commented 1 year ago

Oh, one more formatting issue 🤦‍♂️ . OK, but no exact timeline.

sungam3r commented 1 year ago

@nightroman I think this is not a bug but a decision by design. Argument has its own line only if it has comment or description.

Options:

  1. There should be an option like existing EachDirectiveLocationOnNewLine - maybe EachArgumentDefinitionOnNewLine. This option will force all argument of all fields of all types to be printed on its own lines.
  2. Print all arguments of particular field on new line if at least one argument should be printed on its own line (comment or description exists).
  3. Other suggestions?
sungam3r commented 1 year ago

@nightroman Also try to remove description from some other field, for example, after.

nightroman commented 1 year ago

The design might be questionable... but it is what it is, you might know all the reasons better. Formatting standards are often opinionated and the known source of holy wars :) Options looks like a reasonable way to make clients happy.

nightroman commented 1 year ago

@nightroman Also try to remove description from some other field, for example, after.

Just to see what happens or to "fix/adjust" formatting?

I do not really want to use descriptions (add/remove) in order to adjust formatting. Besides, we do not always author descriptions, some of them are generated. E.g. in the provided example only designators description is "ours". Others, including missing are generated or not generated by HotChocolate that we use for building our GQL.

sungam3r commented 1 year ago

Just to see what happens or to "fix/adjust" formatting?

Just to see what happens.

I provided some options above. Do you have suggestions?

Shane32 commented 1 year ago

I’d suggest option 2 for default behavior. Options would be good also of course.

nightroman commented 1 year ago

I’d suggest option 2 for default behavior. Options would be good also of course.

This looks reasonable to me as well.

Shane32 commented 1 year ago

In general I’d agree with @nightroman that often descriptions are applied selectively in my schemas on an as needed basis. But it would be logical for arguments to either be on one line or separate lines, not a mix. And since adding a description usually requires a line for that, I’d add new lines to all if any did. That’s how I format source code when the [Description] attribute is in play, and is of course my opinion.

sungam3r commented 1 year ago

OK, I'll go on with option 2.

sungam3r commented 1 year ago

358 combines all options

nightroman commented 1 year ago

Awesome! When is the new nuget update expected, approximately? Look forward trying this.

sungam3r commented 1 year ago

After #359 and maybe soon after some changes around #360

sungam3r commented 1 year ago

https://github.com/graphql-dotnet/parser/releases/tag/9.3.0

nightroman commented 1 year ago

@Shane32 @sungam3r Thank you, I have tried 9.3.0, it works exactly as expected!