Fix breaking change messages for root schema type changes/additions #55
Conversation
|
cc @xuorig and @swalkinshaw not exactly sure who I should be tagging but I see y'all in the code—got a bit of a fix here to some minor issues, let me know if there's anything I can do to make it easy to get the fixes in (or if there's anyone else I should be pinging for review) and I'll be happy to do so! |
| @@ -414,11 +434,31 @@ def initialize(old_schema, new_schema) | |||
| end | |||
|
|
|||
| def message | |||
| "Schema mutation root has changed from `#{old_schema.mutation}` to `#{new_schema.mutation}`" | |||
| "Schema mutation root has changed from `#{old_schema.mutation.graphql_name}` to `#{new_schema.mutation&.graphql_name}`" | |||
There was a problem hiding this comment.
Same question about the safe operator usage here
There was a problem hiding this comment.
So here, I think the null safe operator is necessary—this basically covers the case where someone removes a mutation root altogether (instead of changing/renaming it). I just verified that this was also a possible breakage on the schema query/subscription type changed classes, so I added it in there as well. I figured, it would be better to just make these changes null-safe, rather than add a set of SchemaQueryTypeRemoved/SchemaMutationTypeRemoved/SchemaSubscriptionTypeRemoved changes as well.
Let me know if that makes sense to you as well—thank you for taking a look!
There was a problem hiding this comment.
Another alternative is unifying these into "root operation" changes:
RootOperationTypeRemovedRootOperationTypeAddedRootOperationTypeChanged
Messaging could be "Schema root operation type mutation changed from {} to {}`".
What do you think?
There was a problem hiding this comment.
Yeah that makes a lot of sense. Just put those changes in—thanks for the review/suggestion!
| "Default value `#{new_argument.default_value}` was added to argument `#{new_argument.graphql_name}` on field `#{field.path}`" | ||
| else | ||
| if old_argument.default_value? | ||
| "Default value for argument `#{new_argument.graphql_name}` on field `#{field.path}` changed"\ | ||
| " from `#{old_argument.default_value}` to `#{new_argument.default_value}`" | ||
| else | ||
| "Default value `#{new_argument.default_value}` was added to argument `#{new_argument.graphql_name}` on field `#{field.path}`" |
There was a problem hiding this comment.
Also, just a heads up about these default value changes—I updated these because the tests were failing on newer versions of graphql-ruby, that don't rely on :__no_default__ to specify no default argument value.
These changes only really are only necessary if you're running a later version of graphql-ruby—if you'd like me to make these changes separately and also set up a new gemfile specifically to run the test suite on graphql-ruby 2.1, let me know and I'd be happy to pull these changes out of this PR and do that as well.
There was a problem hiding this comment.
Nice improvement 👍 default_value? has actually been around forever so we can safely use that: rmosolgo/graphql-ruby@23a467d
It's just that __no_default__ was only removed in 2.1 I guess.
|
Thanks for the improvement @willlockwood |
As part of our work using graphql-schema_comparator, we've been using the schema comparator in our test suite to add visibility to when developers propose breaking changes to our schema. Recently, we added a mutation root type to a schema that previous didn't have one, and were surprised to see that as a breaking change, and to see the breaking change message using a string representation of the mutation root type itself (meaning that the message itself was not a stable description of the change, but would change every time our test was run):
This PR does two things:
graphql_names of the types, rather than a string representation of the type class, and