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

Fix #1302, camelize type system directives in sdl output #1303

Conversation

michaelcaterisano
Copy link
Contributor

@michaelcaterisano michaelcaterisano commented Feb 29, 2024

This is a follow-up to #1302. I camelized in one too many places and wasn't testing type system directives in the way I intended, e.g. defined in a prototype schema. This PR removes the extra call to Absinthe.Utils.camelize and corrects the test to use a prototype schema.

cc: @benwilson512

@michaelcaterisano michaelcaterisano force-pushed the mc/fix-camelize-directives branch 3 times, most recently from 488855b to 4d769e2 Compare February 29, 2024 01:46
@michaelcaterisano michaelcaterisano force-pushed the mc/fix-camelize-directives branch from 6768aa1 to b77a879 Compare February 29, 2024 01:49
@@ -1991,7 +1991,6 @@ defmodule Absinthe.Schema.Notation do
defp default_name(Schema.DirectiveDefinition, identifier) do
identifier
|> Atom.to_string()
|> Absinthe.Utils.camelize(lower: true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This did succeed in camelCasing the directive in the test below, but it broke a bunch of other things.

Comment on lines -211 to -213
directive :foo_foo do
on :field
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this was a type system directive definition, but we're defining those using a prototype schema. I'm not clear on what kind of directive definition this is.

@michaelcaterisano michaelcaterisano marked this pull request as ready for review February 29, 2024 01:56
Copy link
Contributor

@benwilson512 benwilson512 left a comment

Choose a reason for hiding this comment

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

Thanks for this. The test suite around some of these macro things clearly isn't adequate, thanks for adding things!

@benwilson512 benwilson512 merged commit 5a97423 into absinthe-graphql:main Feb 29, 2024
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.

2 participants