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

Adding the normalize_query_types feature to normalize all generated to CamelCase. #257

Closed
wants to merge 1 commit into from

Conversation

markcatley
Copy link
Contributor

This probably isn't the final solution to this issue but I made this change so that I could continue with what I was implementing. At a minimum, the readme would need to be updated to describe the feature.

I have added the normalize_query_types that normalises types (structs, type aliases, etc) generated by the code-gen to use CamelCase as it is my understanding that this is idiomatic rust. I have done this as a feature as it's potentially a breaking change.

I was having issues with scalars as described in #254 but then noticed that other types were non-idiomatic.

Fixes #254 and fixes #255.

@davidgraeff
Copy link
Contributor

davidgraeff commented Aug 27, 2019

Seems like you solve #255 indeed in a more comprehensive way than I did. I'm not sure if it should be hidden behind a feature flag. It current way breaks generated code and the transformations should always happen imo. I don't think backwards compatibility is an issue because this could never have worked for anyone before.

Why do you introduce an online test and don't follow the _cli tests structure with schema+query files?

@markcatley markcatley force-pushed the master branch 2 times, most recently from ddebd35 to 1925c9b Compare August 27, 2019 08:11
@markcatley
Copy link
Contributor Author

Thank you.

I am definitely happy for it not to be behind a feature flag, however, it is definitely a breaking change. For example, if you go into the github example and turn the feature flag on then you get the following error:

error[E0412]: cannot find type `Uri` in module `super`
  --> src/main.rs:10:10
   |
10 | #[derive(GraphQLQuery)]
   |          ^^^^^^^^^^^^ not found in `super`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0412`.
error: Could not compile `graphql_query_github_example`.

I really wasn't sure how to test this so I created the new example so it can most likely be done in a better way. I don't follow where you're suggesting it should be tested. What is the path to the files you're talking about?

@davidgraeff
Copy link
Contributor

Tests are in graphql_client/tests. I would have expected most of them to be in the _codegen directory like the github test, but I'm not really opinionated.

@tomhoule
Copy link
Member

Sorry for the delayed reply, I'm on vacations at the moment :)

I also think this should be the default. It's unambiguous that we should convert from snake case to camel case. The one in the GitHub example is more puzzling, as it's a renaming from URI to Uri, which is not intuitive. We could think about only doing the case transformation if

  • there is an underscore in the type (excluding the introspection types maybe), or
  • it starts with a lowercase letter

What do you think?

@tomhoule
Copy link
Member

We should also document it (I could do it if you don't have the time).

@davidgraeff
Copy link
Contributor

there is an underscore in the type (excluding the introspection types maybe), or
it starts with a lowercase letter

If you introduce rules for the transformation, there will be people tripping over those one or two odd named types in their schemas. I'd say the transformation should happen all the time, imo.

@tomhoule
Copy link
Member

tomhoule commented Sep 20, 2019

Sorry for the delay! I think we should merge this with the feature at first, we can think about removing it later, I'll try to find the time to rebase and merge (it's great if you want to rebase yourself of course :))

@tomhoule
Copy link
Member

Update: I'm fixing the merge conflicts right now.

@tomhoule
Copy link
Member

Merged in 26db903 and added to the changelog :) We'll probably make this the default eventually. Thanks a lot!

@tomhoule tomhoule closed this Sep 20, 2019
@mathstuf
Copy link
Contributor

mathstuf commented Oct 5, 2019

See #283 for some problems I saw when trying to address #281. PR incoming shortly.

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.

Code gen: Redefinitions Scalar types that begin with a lowercase letter
4 participants