-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add support for Url, Locale, Country Code and Currency scalars … #39942
base: 3.9
Are you sure you want to change the base?
Conversation
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
...eployment/src/main/java/io/quarkus/smallrye/graphql/deployment/SmallRyeGraphQLProcessor.java
Outdated
Show resolved
Hide resolved
2ddf97e
to
e87d979
Compare
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 added some small comments but really this looks very promising and tests are included, which is great.
Is there a reason why it's still draft?
extraScalars.forEach(extraScalar -> | ||
Scalars.registerCustomScalarInSchema( | ||
extraScalar.getGraphQLScalarType().getName(), | ||
extraScalar.getValueClass().getName())); |
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 would avoid lambdas when they don't actually improve readability.
They have a cost.
We tend to avoid them mostly in the runtime part but really here, it's arguably making things less readable.
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.
sure, fixed.
extraScalars.forEach(extraScalar -> | ||
GraphQLScalarTypes.registerCustomScalar( | ||
extraScalar.getGraphQLScalarType().getName(), | ||
extraScalar.getValueClass().getName(), | ||
extraScalar.getGraphQLScalarType())); |
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.
Same here.
…raphql-java-extended-scalars
e87d979
to
2f55b63
Compare
…from graphql-java-extended-scalars