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

Support Short schemas and args #1011

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

Fluxx
Copy link
Contributor

@Fluxx Fluxx commented Aug 23, 2021

I ran into a situation where I had a Short type that I needed a schema for, but Caliban didn't have one. This PR adds one which is implemented as a Scalar, along with an ArgBuilder for it. Few notes:

  • My editor automatically trims training whitespace on save, so there are some whitespace changes. I am happy to remove them if you would like.
  • I didn't see any specific tests for schemas so I didn't add any, but if you point me in the right direction I can add them.
  • Let me know how to update docs. I think I only need to add the row to the table on the schema page, but I wasn't sure if the docs HTML checked in was automatically generated or not.

@ghostdogpr
Copy link
Owner

Looks good! Can you check the Scala 3 test that failed? Maybe it was using an implicit conversion which is no longer allowed?

@Fluxx
Copy link
Contributor Author

Fluxx commented Aug 24, 2021

I looked into it and you were correct. There does not appear to be an implicit short -> big decimal conversion in Scala3. I converted the value to an int first, which then uses the existing int -> big decimal conversion.

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ghostdogpr ghostdogpr merged commit d012343 into ghostdogpr:master Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants