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

More selective detection of breaking/dangerous enum value changes #54

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

jturkel
Copy link
Contributor

@jturkel jturkel commented Sep 30, 2022

This PR fixes #18 by only flagging enum value additions as breaking if the enum is used in output types and only flagging enum value removals as breaking if the enum is used in input types.

I tried out a different way of testing schema changes in GraphQL::SchemaComparator::Diff::SchemaTest that colocates the change path and message and adds testing of the change level. I'm happy to convert the test_changes_kitchensink methood to use this pattern too if you want.

reason: "Removing an enum value will cause existing queries that use this enum value to error."
)
) : Changes::Criticality.non_breaking
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we customize the reason here to explain why? That it's only used in arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call since that will help clients understand the distinction between the breaking and non-breaking cases.

reason: "Adding an enum value may break existing clients that were not " \
"programming defensively against an added case when querying an enum."
)
) : Changes::Criticality.non_breaking
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question here

Copy link
Collaborator

@swalkinshaw swalkinshaw left a comment

Choose a reason for hiding this comment

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

🚀 thank you

Test changes look good. The kitchen sink is harder to work with 👍

@swalkinshaw swalkinshaw merged commit 55336f0 into xuorig:master Sep 30, 2022
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.

Nuance to EnumValueAdded's criticality
2 participants