Pretty format variable definitions for both operations and fragments. #1557
Conversation
| `, | ||
| { experimentalFragmentVariables: true }, | ||
| ); | ||
| expect(print(fragmentWithVariable)).to.equal(dedent` | ||
| fragment Foo($a: ComplexType, $b: Boolean = false) on TestType { | ||
| fragment Foo( | ||
| $a: ComplexType, |
There was a problem hiding this comment.
I'd prefer we just drop the comma entirely. It's totally unnecessary, and causes more arguments about whether we should have a trailing comma or not. But as this PR is about bringing fragment variable definitions to parity with query variable definitions, I won't block us on that.
There was a problem hiding this comment.
I can fix the test, that is fine.
src/language/printer.js
Outdated
| if (args.length === 1) { | ||
| return '(' + args[0] + ')'; | ||
| } | ||
| return '(\n' + indent(join(args, ',\n')) + '\n)'; |
There was a problem hiding this comment.
We were only using the comma due to having everything on one line. A newline is a perfectly acceptable variable definition separator, just as it is for fields inside a document.
|
@adek05 @mjmahone Single line approach works for simple cases but not for something like this: VS I think single argument per line is safer option for now but in long run we need to implement something simular to prettier algorithm. |
|
@IvanGoncharov I wasn't sure whether single variable should be special-cased, but your example convinced me it should not be. |
adek05
force-pushed
the
fix-arguments-formatting
branch
2 times, most recently
from
1d266c4 to
917eefc
Compare
When there is more than a couple argument values, pretty formatting
isn't doing a good job.
Instead I would like to take the following approach:
When there is one argument, print as is, meaning:
query Q ($arg: Type = Default) {
When there is more than one do the following:
query Q (
$arg1: Type = Default,
$arg2
) {
An alternative would be to have closing bracket be in the same line
as final argument. I am fine with either, as changes which introduce
another argument on last position always touch two lines - traiiling
comma will have to be added.
adek05
force-pushed
the
fix-arguments-formatting
branch
2 times, most recently
from
202804c to
eb60915
Compare
|
Changes incorporated, let me know if there is anything else you would like to see or good to go. |
| query ($foo: TestType) @testDirective { | ||
| query ( | ||
| $foo: TestType | ||
| ) @testDirective { |
There was a problem hiding this comment.
This feels worse to me. Why wrap like this?
| if (!args || args.length === 0) { | ||
| return ''; | ||
| } | ||
| return '(\n' + indent(join(args, '\n')) + '\n)'; |
There was a problem hiding this comment.
Happy to see this function factored out to avoid duplication, but I think the additional new lines aren't helping legibility
|
Let's remember that this AST printer does not aim to be prettier, it should print well when possible, but I don't think we can attempt full support for edge cases like @IvanGoncharov's without implementing parts of Prettier's algorithm. Prettier already has support for GraphQL, which should be used for more complex cases. However, I also want to avoid a "fully expanded" printing of the AST because of these limitations. I think that hurts legibility more than it helps it. For instance simple cases like the example below, one line is preferable. Would it be possible to compromise by using the length of the child strings to determine if they should be a one line or multi-line print? Also, why only variable definitions? argument definitions, arguments on fields, elements of input objects, lists, are also examples of things currently always printed as a comma-separated list but could suffer from the same issue. I'd love to see a solution that creates consistency instead of a one-off for variable definitions alone. |
|
Extracted as #3230 |
When there is more than a couple argument values, pretty formatting
isn't doing a good job.
Instead I would like to take the following approach:
When there is one argument, print as is, meaning:
query Q ($arg: Type = Default) {When there is more than one do the following:
An alternative would be to have closing bracket be in the same line
as final argument. I am fine with either, as changes which introduce
another argument on last position always touch two lines - trailing
comma will have to be added.
Edited tests to make sure everything works as expected.