Skip to content

Unknow other variant for fragment #373

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

Closed
wants to merge 4 commits into from
Closed

Unknow other variant for fragment #373

wants to merge 4 commits into from

Conversation

visd0m
Copy link
Contributor

@visd0m visd0m commented Jun 27, 2021

Tries to solve the issue.

Add the option fragments-other-variant to allow the generation of an Unkown "serde other" variant in the Enum generated by fragments.

The option can be set as macro attributes or by flag if using the graphql-client-cli.

As default the option is disabled.

@visd0m visd0m marked this pull request as ready for review June 28, 2021 10:01
Copy link
Member

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

I did a very quick read and it looks like the PR is on the right path 👍

Sorry for the small merge conflicts because of #371

@tomhoule
Copy link
Member

I'll try to find time soon for a proper review. Thanks for the PR!

@damjack
Copy link

damjack commented Jun 29, 2021

Hi @tomhoule, I have a doubt.
Handling the Enum types the lib adds Other(String) variant as default (no need to set any flags).

What about doing the same thing for the union/interface types making it the default behaviour.
This ☝️ would make it possible to remove the new flag --fragments-other-variant and simplify the code.

@tomhoule
Copy link
Member

I have only a pretty distant memory of what the generated code looks like, but that sounds very reasonable.

@visd0m
Copy link
Contributor Author

visd0m commented Jun 30, 2021

@tomhoule I made a second commit which implements the feature as suggested by @damjack.
You can find:

  • in the first commit the changes proposed to handle the feature through a new option
  • in the second commit the changes proposed to handle the feature as default behavior, no options needed

In this way it is easy to see the two versions and decide which one to carry on

@tomhoule
Copy link
Member

tomhoule commented Jul 4, 2021

Hi @visd0m , I just had a look at both commits, and I think we should have the opt-in option at first (first commit). In general the commit looks good 👍 I'll cut a release now, so we can merge without worrying too much after that and have time to test.

@visd0m
Copy link
Contributor Author

visd0m commented Jul 4, 2021

Hi @visd0m , I just had a look at both commits, and I think we should have the opt-in option at first (first commit). In general the commit looks good 👍 I'll cut a release now, so we can merge without worrying too much after that and have time to test.

@tomhoule I reverted the last commit and restore the first changes.
Just wondering if after the merge we can think about another release 😬 ? (hoping to use this without pointing to master)

@tomhoule
Copy link
Member

tomhoule commented Jul 5, 2021

Yes, we can definitely cut another release as soon as I find some time for graphql-client again, hopefully soon. I'd probably take the opportunity to also change graphql_client_codegen to not depend on syn and quote anymore too.

@tomhoule tomhoule closed this Jul 7, 2021
@tomhoule tomhoule deleted the branch graphql-rust:master July 7, 2021 07:10
@tomhoule
Copy link
Member

tomhoule commented Jul 7, 2021

@visd0m I changed the main branch for the repo from master to main and that automatically closed the PR. Please re-open the PR to the main branch, sorry for the unintended problem.

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.

3 participants