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

feat: Support @GQLDefault #1043

Merged
merged 13 commits into from
Sep 15, 2021
Merged

feat: Support @GQLDefault #1043

merged 13 commits into from
Sep 15, 2021

Conversation

frekw
Copy link
Collaborator

@frekw frekw commented Sep 14, 2021

Hi!

This hopefully closes #1010.

As discussed, I've had some trouble with validating the default values. I think this approach is sort of OK, but am happy to try other paths that might lead to less code duplication.

The error messages aren't great right now because they're lacking some context. Will improve them before marking this PR as ready to go!

@frekw frekw marked this pull request as ready for review September 14, 2021 21:15
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.

Imports management is THE reason I can't use Metals 😆

Can you add documentation in https://github.com/ghostdogpr/caliban/blob/master/vuepress/docs/docs/schema.md#annotations ?

Looks great otherwise!

core/src/main/scala/caliban/validation/DefaultValue.scala Outdated Show resolved Hide resolved
core/src/main/scala/caliban/validation/Validator.scala Outdated Show resolved Hide resolved
import caliban.Value.FloatValue.FloatNumber
import caliban.InputValue.ObjectValue

object DefaultValue {
Copy link
Owner

Choose a reason for hiding this comment

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

DefaultValueValidator? To keep the naming consistent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Naming is my weakness :D

945e1e4

core/src/main/scala/caliban/validation/DefaultValue.scala Outdated Show resolved Hide resolved
IO.fromEither(Parser.parseInputValue(v))
.mapError(e =>
ValidationError(
s"$errorContext failed to parse default value: ${e.msg}",
Copy link
Owner

Choose a reason for hiding this comment

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

Is there always a line break at the end of the error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, no I don't think so, but there are two messages, msg and explanatoryText.

core/src/main/scala/caliban/validation/DefaultValue.scala Outdated Show resolved Hide resolved
core/src/main/scala/caliban/validation/DefaultValue.scala Outdated Show resolved Hide resolved
@frekw
Copy link
Collaborator Author

frekw commented Sep 15, 2021

Added docs in 37dd3a1. 👍

Imports management is THE reason I can't use Metals 😆

I've learnt to live with it. It being automatic is just too big of a convenience 😅. But it seems as if it hadn't run correctly on all files, so I've cleaned the imports up.

Looks great otherwise!
🙇

@frekw frekw requested a review from ghostdogpr September 15, 2021 07:02
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.

Looks all good, thanks!

@ghostdogpr ghostdogpr merged commit 54eba28 into ghostdogpr:master Sep 15, 2021
@frekw frekw deleted the feat/gqldefault branch September 15, 2021 08:11
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.

Default argument value support
2 participants