Skip to content

Conversation

@jmpunkt
Copy link
Contributor

@jmpunkt jmpunkt commented Apr 12, 2020

Implements derive macros for tagged enums which are translated into GraphQLUnion types. The following features are provided

  • The macro implements GraphQLAsyncType.
  • If all variants of the tagged enum have different types, then the derive macro implements std::convert::From<T> converter for these types.

Fixes: #586

Copy link
Member

@LegNeato LegNeato 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! Thank you so much for the PR, we always love new contributors writing high-quality code. 🥂 🎉

A couple of small things:

If you are up to it interfaces are very similar to unions in GraphQL and that would be a great next place to contribute...though they are a bit more complicated than unions.

@jmpunkt jmpunkt requested a review from LegNeato April 13, 2020 17:34
@jmpunkt
Copy link
Contributor Author

jmpunkt commented Apr 13, 2020

Thanks for the review. Everything you mentioned is fixed. During the testing i encountered some issues. Therefore, skip and documentation annotations are disabled for enumeration fields. Here are some thoughts about these decisions and the implementation. If something is unclear or wrong, let me know.

  • Documentation of variations is not possible in GraphQL? (not 100% sure)
  • Skipping fields works in general but throws panics during runtime. For example, if a query returns a skipped variant of the enum. The derive macro should provide additional safety (compared to the other union implementation). If the user requires this behavior he can fallback to the other union implementation.
  • Currently the std::convert::Into ensures that each variant has a unique type. Due to the GraphQL specification this must be done? (not 100% sure)

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

It looks like at least in js unions can have descriptions for the entire thing: https://graphql.org/graphql-js/type/#graphqluniontype and it appears that the union types can as well. But, that is Apollo and not the official spec. So let's leave descriptions and skip out.

jmpunkt added 2 commits April 14, 2020 14:07
Simplifies code and provides the idea of using
`util::GraphQLTypeDefinitionField` for different types than objects.
@jmpunkt
Copy link
Contributor Author

jmpunkt commented Apr 14, 2020

This PR should be ready to merge. Applied your feedback and added some minor tweaks.

@jmpunkt jmpunkt requested a review from LegNeato April 14, 2020 21:09
@LegNeato LegNeato merged commit a05f4e5 into graphql-rust:master Apr 15, 2020
@LegNeato
Copy link
Member

Awesome, thank you again!

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.

graphql_union from tagged union of structs

2 participants