-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
273 - use builder pattern in GraphQL top level object #276
273 - use builder pattern in GraphQL top level object #276
Conversation
@@ -5,14 +5,14 @@ | |||
public class Assert { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not related but inline assignment and null checking is super handy
@@ -66,7 +119,7 @@ public ExecutionResult execute(String requestString, String operationName, Objec | |||
RecognitionException recognitionException = (RecognitionException) e.getCause(); | |||
SourceLocation sourceLocation = new SourceLocation(recognitionException.getOffendingToken().getLine(), recognitionException.getOffendingToken().getCharPositionInLine()); | |||
InvalidSyntaxError invalidSyntaxError = new InvalidSyntaxError(sourceLocation); | |||
return new ExecutionResultImpl(Arrays.asList(invalidSyntaxError)); | |||
return new ExecutionResultImpl(Collections.singletonList(invalidSyntaxError)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smidge more efficient and IDEA inspections (which I trust mostly) says to do it.
Broken window theory in code and all...
@@ -94,7 +94,9 @@ public class HelloWorld { | |||
GraphQLSchema schema = GraphQLSchema.newSchema() | |||
.query(queryType) | |||
.build(); | |||
Map<String, Object> result = (Map<String, Object>) new GraphQL(schema).execute("{hello}").getData(); | |||
|
|||
GraphQL graphQL = GraphQL.newObject(schema).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GraphQL.newGraphQL() I think is more in line with the other builder factory names. (newObject() is already "taken" by GraphQLObjectType)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -29,16 +29,69 @@ | |||
|
|||
private static final Logger log = LoggerFactory.getLogger(GraphQL.class); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should take into account the mutation strategy selection, which is already released https://github.com/graphql-java/graphql-java/pull/240/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I am updating that now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok now have updated the readme as well to give examples...plus made it the examples compile time safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready to go
.query(queryType) | ||
.build(); | ||
|
||
GraphQL graphQL = GraphQL.newObject(schema).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default its looks the other builders and uses SimpleStrategy for execution. You can then use your own via builder methods
@@ -29,16 +29,69 @@ | |||
|
|||
private static final Logger log = LoggerFactory.getLogger(GraphQL.class); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok now have updated the readme as well to give examples...plus made it the examples compile time safe.
@@ -0,0 +1,179 @@ | |||
package readme; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way the readme will stay correct with the minimum effort of cut and paste
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok ready to go
@@ -94,7 +94,9 @@ public class HelloWorld { | |||
GraphQLSchema schema = GraphQLSchema.newSchema() | |||
.query(queryType) | |||
.build(); | |||
Map<String, Object> result = (Map<String, Object>) new GraphQL(schema).execute("{hello}").getData(); | |||
|
|||
GraphQL graphQL = GraphQL.newObject(schema).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
#273 - to allow consistency with other parts of the API and extensibility in the future, the builder pattern should be used on the GraphQL object