-
Notifications
You must be signed in to change notification settings - Fork 47
feat(compiler): implement @oneOf input objects #1030
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
base: main
Are you sure you want to change the base?
Changes from all commits
abdbf3b
0ffec14
80ad7fe
8987909
e8a9e22
0085448
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -292,8 +292,39 @@ pub(crate) enum DiagnosticData { | |
| "{describe} cannot be named `{name}` as names starting with two underscores are reserved" | ||
| )] | ||
| ReservedName { name: Name, describe: &'static str }, | ||
| #[error("`{coordinate}` field of a @oneOf input object must be nullable")] | ||
| OneOfInputObjectFieldNonNull { | ||
| coordinate: TypeAttributeCoordinate, | ||
| definition_location: Option<SourceSpan>, | ||
| }, | ||
|
Comment on lines
+295
to
+299
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds like an existing
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think If the underlying ask is "make the name less Does that get at the root or is it something else?? |
||
| #[error( | ||
| "@oneOf input object `{name}` must specify exactly one key, but {provided} {} given", | ||
| if *provided == 1 { "was" } else { "were" } | ||
| )] | ||
| OneOfInputObjectWrongNumberOfFields { name: Name, provided: usize }, | ||
|
Comment on lines
+300
to
+304
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather have these be non-specific to oneOf. Perhaps this can be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure the All the existing Two alternatives worth considering:
what's your take? same as before? |
||
| #[error("`{name}.{field}` value for @oneOf input object must be non-null")] | ||
| OneOfInputObjectNullField { name: Name, field: Name }, | ||
|
Comment on lines
+305
to
+306
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also sounds like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a clean middle ground here, I think. Your underlying point that this shouldn't be Would you be in favor of a new generic variant like Want me to take a swing at that? |
||
| #[error("`{coordinate}` field of a @oneOf input object must not have a default value")] | ||
| OneOfInputObjectFieldHasDefault { | ||
| coordinate: TypeAttributeCoordinate, | ||
| default_location: Option<SourceSpan>, | ||
| }, | ||
|
Comment on lines
+307
to
+311
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like one of the comments above, this should also be named independantly of oneOf. Perhaps, just
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair. will swap to |
||
| #[error( | ||
| "variable `${variable}` is of type `{variable_type}` \ | ||
| but must be non-nullable to be used for @oneOf input object `{name}` field `{field}`" | ||
| )] | ||
| OneOfInputObjectNullableVariable { | ||
| name: Name, | ||
| field: Name, | ||
| variable: Name, | ||
| variable_type: Node<ast::Type>, | ||
| }, | ||
|
Comment on lines
+312
to
+321
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be an existing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tried this but the field shape doesn't quite line up. Notes below!
Three(?+) paths: a. Keep Thoughts? Mildly lean toward "a" since the location semantics seem genuinely different but happy to follow b/c if you'd rather see consolidation. Which do you prefer? |
||
| } | ||
|
|
||
| /// Shared help text for the two @oneOf schema-definition errors. | ||
| const ONE_OF_FIELD_REQUIREMENTS: &str = | ||
| "Fields of a @oneOf input object must all be nullable and must not have default values."; | ||
|
|
||
| impl DiagnosticData { | ||
| pub(crate) fn report(&self, main_location: Option<SourceSpan>, report: &mut CliReport) { | ||
| match self { | ||
|
|
@@ -721,6 +752,66 @@ impl DiagnosticData { | |
| DiagnosticData::ReservedName { name, .. } => { | ||
| report.with_label_opt(name.location(), "Pick a different name here"); | ||
| } | ||
| DiagnosticData::OneOfInputObjectFieldNonNull { | ||
| coordinate, | ||
| definition_location, | ||
| } => { | ||
| report.with_label_opt( | ||
| *definition_location, | ||
| format_args!("field `{coordinate}` defined here"), | ||
| ); | ||
| report.with_label_opt(main_location, "remove the `!` to make this field nullable"); | ||
| report.with_help(ONE_OF_FIELD_REQUIREMENTS); | ||
| } | ||
| DiagnosticData::OneOfInputObjectWrongNumberOfFields { name, provided } => { | ||
| report.with_label_opt( | ||
| main_location, | ||
| format_args!( | ||
| "{provided} {} provided", | ||
| if *provided == 1 { | ||
| "field was" | ||
| } else { | ||
| "fields were" | ||
| } | ||
| ), | ||
| ); | ||
| report.with_help(format_args!( | ||
| "@oneOf input object `{name}` requires exactly one non-null field." | ||
| )); | ||
| } | ||
| DiagnosticData::OneOfInputObjectNullField { name, field } => { | ||
| report.with_label_opt(main_location, "this value is null"); | ||
| report.with_help(format_args!( | ||
| "@oneOf input object `{name}` field `{field}` must be non-null." | ||
| )); | ||
| } | ||
| DiagnosticData::OneOfInputObjectFieldHasDefault { | ||
| coordinate, | ||
| default_location, | ||
| } => { | ||
| report.with_label_opt( | ||
| *default_location, | ||
| format_args!("default value for `{coordinate}` defined here"), | ||
| ); | ||
| report.with_label_opt(main_location, "remove the default value"); | ||
| report.with_help(ONE_OF_FIELD_REQUIREMENTS); | ||
| } | ||
| DiagnosticData::OneOfInputObjectNullableVariable { | ||
| name, | ||
| field, | ||
| variable, | ||
| variable_type, | ||
| } => { | ||
| report.with_label_opt( | ||
| main_location, | ||
| format_args!( | ||
| "variable `${variable}` has type `{variable_type}`, which is nullable" | ||
| ), | ||
| ); | ||
| report.with_help(format_args!( | ||
| "use `{variable_type}!` to make this variable non-nullable for @oneOf input object `{name}` field `{field}`." | ||
| )); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
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 comment as with apollo-smith: we usually bump these in a release PR
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.
Yeah, I can revert that. I had this for local testing of something.