Closed
Conversation
leebyron
added
the
✏️ Editorial
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
leebyron
force-pushed
the
lee/coerce-values
branch
from
00aeec5 to
695b9dd
Compare
An alternative to #1056 which fully removes the `hasValue` variable in favor of hopefully more clear conditions
leebyron
force-pushed
the
lee/coerce-values
branch
from
695b9dd to
6da555d
Compare
mjmahone
reviewed
Jul 3, 2025
| - Otherwise if {variableDefinition} has a default value, let it be | ||
| {defaultValue}: | ||
| - If {defaultValue} is {null}: | ||
| - Assert: {variableType} is not a Non-Nullable type. |
Contributor
There was a problem hiding this comment.
Why is this language of Assert different from the
If {variableType} is a Non-Nullable type, raise a request error.
above?
Collaborator
Author
There was a problem hiding this comment.
default values have gone through request validation, so an error would have been thrown at validation time. I believe we use Assert in other places where that should be true.
It's a good flag though that this should be more clear. It would be nice for all asserts to link back to the validation rules which allow them to be assertions instead of conditions
Collaborator
Author
|
Closing in favor of #1179 |
leebyron
added
the
🌱 Superseded (RFC X)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An alternative to #1056 which fully removes the
hasValuevariable in favor of hopefully more clear conditionsThe original intent of tracking
hasValuewas to avoid duplication of logic in places where once we've gotten the "value" from whatever source (value, variable, etc) then the rest of the steps are the same. However that's proven to be fragile and hard to read, since values from different sources are in fact handled differently.This instead removes the concept of
hasValueby instead just using more specific conditions like "If __ has an entry named ___, then let ___ be its value"For future -- I'd love to overhaul the entire "input coercion" to be strict algorithms instead of prose, which seems particularly important for being crisp about what to do with variables.