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

Allow maps as arguments in macros #1226

Merged

Conversation

kzlsakal
Copy link
Contributor

@kzlsakal kzlsakal commented Feb 3, 2023

This is one approach to possibly solve the issue #1225

My main goal is to support passing a map in the arguments for a directive that is wrapped in an extend macro.

Copy link
Contributor

@maartenvanvliet maartenvanvliet left a comment

Choose a reason for hiding this comment

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

Good catch!

I think the best place to convert the AST to maps would be in the Schema.Notation module. This to prevent the AST from leaking into other parts of Absinthe.

We can limit support to literal maps only for now. This would most closely match the SDL notation.

Note that this issue also happens elsewhere: #1024

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.

I think the best place to convert the AST to maps would be in the Schema.Notation module.

Agreed, this is a notation concern not a blueprint concern.

@kzlsakal kzlsakal force-pushed the kzlsakal/maps-as-macro-arguments branch from 67ce2c9 to b326789 Compare March 30, 2023 21:23
@kzlsakal
Copy link
Contributor Author

Thanks for the feedback. I moved the logic to Notation as you both recommended.

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.

Looks great, thanks!

@benwilson512 benwilson512 merged commit 1414fe1 into absinthe-graphql:master May 13, 2023
@kzlsakal kzlsakal deleted the kzlsakal/maps-as-macro-arguments branch May 13, 2023 18:33
@hauleth
Copy link

hauleth commented Jun 20, 2023

When this will be released?

@hauleth
Copy link

hauleth commented Jun 20, 2023

This cause Inspect.Error to be written in generated SDL file while command happily succeeds.

@benwilson512
Copy link
Contributor

Hey @hauleth please file an issue with the relevant info on this.

@hauleth
Copy link

hauleth commented Jun 21, 2023

I found more info - it works, but when the keys are binaries, not when keys are atoms. I will try to post more info when I will find time to spare.

@maartenvanvliet
Copy link
Contributor

diff --git a/lib/absinthe/schema/notation/sdl_render.ex b/lib/absinthe/schema/notation/sdl_render.ex
index baac5a24..35ea517d 100644
--- a/lib/absinthe/schema/notation/sdl_render.ex
+++ b/lib/absinthe/schema/notation/sdl_render.ex
@@ -408,7 +408,7 @@ defmodule Absinthe.Schema.Notation.SDL.Render do
   end
 
   defp render_value(%Blueprint.Input.Field{name: name, input_value: value}),
-    do: concat([name, ": ", render_value(value)])
+    do: concat([to_string(name), ": ", render_value(value)])
 
   defp render_value(%{value: value}),
     do: to_string(value)

The Inspect.concat/1 expects binaries so it fails here when it encounters an atom. Above patch fixes it but perhaps it's better to cast to a string or fail in the expand_ast call as pretty much anything can be the key of a map.

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.

4 participants