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

Option for externally defined enums #359

Merged
merged 2 commits into from Mar 9, 2021

Conversation

jakmeier
Copy link
Contributor

@jakmeier jakmeier commented Mar 7, 2021

Allow to deserialize GraphQL enums into user defined Rust enums, as described in #358

Allow to deserialize GraphQL enums into user defined Rust enums.
@jakmeier
Copy link
Contributor Author

jakmeier commented Mar 7, 2021

Here is a first suggestion to add the feature requested in #358. The implementation is very simple. Just don't generate enums if it is in the specified list of external enums. Most of the changes are for adding the option and parsing it. Looking forward to your input on this when you have time to take a look.

Example usage:

#[derive(GraphQLQuery)]
#[graphql(
    schema_path = "api/schema.json",
    query_path = "api/queries/buildings_query.graphql",
    extern_enums("BuildingType")
)]
pub struct BuildingsQuery;

I tested this with my game and 7 queries which use 1-2 enums each. It works as intended for me. As expected, I was able to remove hundreds LOC of boiler-plate code as a result of this. :-)

This is intentionally a minimal change to get the feature done. I guess from my point of view, this is all I really need. But I'm happy to add more bells and whistles, like the option to rename the type name, if that is desired.

What I'm most uncertain about in the current implementation is the error handling. I tried to keep it in line with existing code. It seems that errors due to incorrectly formatted attributes are ignored silently and compilation proceeds as if the attributes were not defined. This is what the new extern_enums attribute does now, too. Let me know if you prefer something else.

Also, please let me know if anything else I should do for the PR, such as writing changelogs or documentation.

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.

Looks good! A test would be great if you have time for it, otherwise I'll write one next time I have a few hours to work on graphql-client.

all_used_types.enums(query.schema).map(move |(_id, r#enum)| {
all_used_types.enums(query.schema)
.filter(move |(_id, r#enum)| !options.extern_enums().contains(&r#enum.name))
.map(move |(_id, r#enum)| {
Copy link
Member

Choose a reason for hiding this comment

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

I assume this just works because the generated modules already use super::*?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it very much relies on that. It's simple and it works. :) Unless you think use super::* might be removed in the future, I don't really see this as a problem. But happy to look into alternatives if you think there could be a cleaner solution.

@jakmeier
Copy link
Contributor Author

jakmeier commented Mar 8, 2021

Looks good! A test would be great if you have time for it, otherwise I'll write one next time I have a few hours to work on graphql-client.

Absolutely, I will create a test shortly

@jakmeier
Copy link
Contributor Author

jakmeier commented Mar 8, 2021

I added two basic tests.

Checking the CI again, there are no new issues.

However, I just got a bit confused with this error that started failing on master in the daily run from 3 days ago:

 failures:

---- tests::schema_with_keywords_works stdout ----
thread 'tests::schema_with_keywords_works' panicked at 'assertion failed: generated_code.contains(\"extern_\")', graphql_client_codegen/src/tests/mod.rs:32:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrac

Really looks like something that I could have caused with external_enum! But since master fails with the same error, it can't be my change. I can reproduce it locally when updating to nightly-2021-03-06 but it was fine before.

@tomhoule
Copy link
Member

tomhoule commented Mar 9, 2021

The tests look good! I think we can merge this, I sadly don't have time to invest in looking at what's wrong in CI now, but I definitely will when I have a few hours to spend on graphql-client. For now we can totally merge this, thanks a lot for the idea and the contribution!

@tomhoule tomhoule merged commit da80b9a into graphql-rust:master Mar 9, 2021
@jakmeier
Copy link
Contributor Author

jakmeier commented Mar 9, 2021

Awesome, thanks for the swift responses and merging!

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.

None yet

2 participants