Skip to content
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

Draft
wants to merge 1 commit into
base: 3.9
Choose a base branch
from

Conversation

cristianonicolai
Copy link
Contributor

…from graphql-java-extended-scalars

Copy link

quarkus-bot bot commented Apr 8, 2024

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should not end up with ellipsis (make sure the title is complete)
  • title should preferably start with an uppercase character (if it makes sense!)
  • title should not start with chore/docs/feat/fix/refactor but be a proper sentence

This message is automatically generated by a bot.

@cristianonicolai cristianonicolai changed the title feat: Add support for Url, Locale, Country Code and Currency scalars … Add support for Url, Locale, Country Code and Currency scalars … Apr 8, 2024
Copy link
Member

@gsmet gsmet left a 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?

Comment on lines 334 to 337
extraScalars.forEach(extraScalar ->
Scalars.registerCustomScalarInSchema(
extraScalar.getGraphQLScalarType().getName(),
extraScalar.getValueClass().getName()));
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, fixed.

Comment on lines 41 to 45
extraScalars.forEach(extraScalar ->
GraphQLScalarTypes.registerCustomScalar(
extraScalar.getGraphQLScalarType().getName(),
extraScalar.getValueClass().getName(),
extraScalar.getGraphQLScalarType()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@cristianonicolai
Copy link
Contributor Author

@gsmet I left it as draft because tests should actually fail. There is still an issue with serializing these new classes using JsonB whne they dont have a default constructor. I'm having a chat with @jmartisk about how to overcome this limitation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants