Changes in schema directive printing AND the comment formats #1609
Conversation
…ent formats used
| @@ -62,17 +62,27 @@ | |||
|
|
|||
| static { | |||
There was a problem hiding this comment.
Unrelated comment.
Is there a reason for this to be in a static block versus just building it directly into the static variable using the builder, for example:
static final DirectiveDefinition DEPRECATED_DIRECTIVE_DEFINITION = DirectiveDefinition.newDirectiveDefinition().name("deprecated")
.directiveLocation(createLocation(DirectiveLocation.FIELD_DEFINITION))
...
.inputValueDefinition(...)
.build();
There was a problem hiding this comment.
yeah not sure - looks like it could have been a chained builder call.
| @@ -113,6 +115,24 @@ class SchemaPrinterTest extends Specification { | |||
| argStr == "(arg1: [Int!] = 10, arg2: String, arg3: String = \"default\")" | |||
| } | |||
|
|
|||
| def "argsString_comments"() { | |||
| def argument1 = new GraphQLArgument("arg1", "A multiline\ncomment", list(nonNull(Scalars.GraphQLInt)), 10) | |||
There was a problem hiding this comment.
Do we support new line characters for window systems? \r\n
There was a problem hiding this comment.
Nominally yes - but we build and dev on Mac / Linux only. That is we dont run the tests on Windows per se.
I have forgotten my \n rules on Windows but for some reason I recall it working as expected in many circumstances
| out.format(" %s%s\n", enumValueDefinition.getName(), directivesString(GraphQLEnumValueDefinition.class, enumValueDefinition.getDirectives())); | ||
| List<GraphQLDirective> enumValueDirectives = enumValueDefinition.getDirectives(); | ||
| if (enumValueDefinition.isDeprecated()) { | ||
| enumValueDirectives = addDeprecatedDirectiveIfNeeded(enumValueDirectives); |
There was a problem hiding this comment.
I see you have this private method to help adding the deprecated directive to the list if it does not exist. Is there a reason why this is a list instead of a set in which case this private method wouldn't be needed.
| @@ -458,7 +511,7 @@ private static String printAst(Object value, GraphQLInputType type) { | |||
|
|
|||
| // when serializing a GraphQL schema using the type system language, a | |||
| // schema definition should be omitted if only uses the default root type names. | |||
| boolean needsSchemaPrinted = options.includeSchemaDefinition; | |||
| boolean needsSchemaPrinted = options.isIncludeSchemaDefinition(); | |||
There was a problem hiding this comment.
Any reason for adding the is at the start, seems to flow easier without it.
There was a problem hiding this comment.
one was a field - this is now the public method. Not sure why I changed it. Consistency elsewhere?? Not sure
| private boolean hasDeprecatedDirective(List<GraphQLDirective> directives) { | ||
| return directives.stream() | ||
| .filter(this::isDeprecatedDirective) | ||
| .count() == 1; |
There was a problem hiding this comment.
Considering this is a list and not a set, can we ever have count > 1, in which case should this be count() >= 1 to work for all these scenarios? Or have we built enough invariants for this class that this shouldn't happen?
There was a problem hiding this comment.
Hmm good point - the rest of the code base puts List<GraphQLDirective> everywhere and hence its influenced this code as well
I think other parts of the code stop multiple named directives
| } | ||
|
|
||
| private AstDescriptionAndComments getDescriptionAndComments(Object descriptionHolder) { | ||
| private String getDescription(Object descriptionHolder) { |
There was a problem hiding this comment.
Can this be a static function? does it need references to this?
There was a problem hiding this comment.
could be - but almost no other method is static in this class - so its consistent in that regard
Co-Authored-By: Jaiden Ashmore <jaidenkyleashmore@gmail.com>
Co-Authored-By: Jaiden Ashmore <jaidenkyleashmore@gmail.com>
Co-Authored-By: Jaiden Ashmore <jaidenkyleashmore@gmail.com>
See #1587
Will now ONLY show descriptions (and never captured comments other than ones turned into descriptions by SchemaGen