Enable generating graphql client code by cli #122

h-michael · 2018-09-17T15:20:26Z

Implement #43

implement feature

h-michael · 2018-09-17T15:21:55Z

@tomhoule
How do you think this implementation?
I am glad if you give me your opinion.

tomhoule

Very nice! I think we can merge this once the comments are addressed. Since the codegen is the same as the derive and that is tested, I don't think we need to test the cli (it's hard), if you can and want to feel free to do it of course but don't worry about it otherwise :)

Have you tried this on your projects? I'll try to try it live too to see how it works. I'll open an issue about globbing because it's something that is convenient in apollo-codegen (you can dump all the types for src/**/*.graphql for example into one module).

tomhoule · 2018-09-18T05:47:45Z

graphql_client_cli/src/main.rs

+        #[structopt(short = "a", long = "additional_derives")]
+        additional_derives: Option<String>,
+        #[structopt(short = "d", long = "deprecation_strategy",)]
+        deprecation_strategy: Option<u8>,


For additional derives and deprecation strategy, I prefer hyphens instead of underscores for the long names, so we can write graphql-client-cli generate --additional-derives=Serialize,Clone instead of --additional_derives.

For the deprecation strategy options could we take a string instead? I think --deprecation-strategy=allow is more intuitive than --deprecation-strategy=0.

OK I'll fix it later :)

There's no rush :)

tomhoule · 2018-09-18T05:49:48Z

graphql_client_codegen/src/lib.rs

+pub struct GraphQLClientDeriveOptions {
+    pub selected_operation: String,
+    pub additional_derives: Option<String>,
+    pub deprecation_strategy: Option<deprecation::DeprecationStrategy>,


Since there is default (Warn) I think deprecation_strategy does not need to be an option, what do you think?

I actually think there is a "bug". The default on the struct is "allow" but in usage is "warn". I'll make them the same in a PR in the morning.

I think deprecation_strategy does not need to be an option, what do you think?

Now, we determine default value in the method, generate_module_token_stream.
So I let this field option.

tomhoule · 2018-09-18T05:54:56Z

graphql_client_cli/src/main.rs

+        query_path: PathBuf,
+        #[structopt(parse(from_os_str))]
+        schema_path: PathBuf,
+        selected_operation: String,


I think we could make selected_operation optional (in the derive implementation we just take the first one by default if no operation is selected).

@tomhoule
I think selected_option is also used as module name.
So we couldn't let selected_option be optional.

h-michael · 2018-09-18T06:01:36Z

Have you tried this on your projects?

Not yet, but generated test deriven file. I'll try it after done my business : )

h-michael · 2018-09-18T10:50:41Z

@tomhoule
I fixed the parts you pointed out and reply to comments.

h-michael · 2018-09-19T00:33:20Z

I've generated rust code as sample.
$ graphql-client generate test.graphql schema.json TestStrcut test.rs

pub mod test_strcut {
    #![allow(non_camel_case_types)]
    #![allow(non_snake_case)]
    #![allow(dead_code)]
    use serde;
    pub const QUERY: &'static str = "query loginUser {\n  viewer {\n    login\n  }\n}\n\n";
    pub const OPERATION_NAME: &'static str = "TestStrcut";
    #[allow(dead_code)]
    type Boolean = bool;
    #[allow(dead_code)]
    type Float = f64;
    #[allow(dead_code)]
    type Int = i64;
    #[allow(dead_code)]
    type ID = String;
    #[derive(Deserialize)]
    #[doc = "A user is an individual\'s account on GitHub that owns repositories and can make new content."]
    pub struct RustLoginUserViewer {
        #[doc = "The username used to login."]
        pub login: String,
    }
    #[derive(Serialize)]
    pub struct Variables;
    #[derive(Deserialize)]
    pub struct ResponseData {
        #[doc = "The currently authenticated user."]
        pub viewer: RustLoginUserViewer,
    }
}
impl<'de> ::graphql_client::GraphQLQuery<'de> for TestStrcut {
    type Variables = test_strcut::Variables;
    type ResponseData = test_strcut::ResponseData;
    fn build_query(
        variables: Self::Variables,
    ) -> ::graphql_client::GraphQLQueryBody<Self::Variables> {
        ::graphql_client::GraphQLQueryBody {
            variables,
            query: test_strcut::QUERY,
            operation_name: test_strcut::OPERATION_NAME,
        }
    }
}

h-michael · 2018-09-19T00:37:42Z

BTW, I'm not good at English, so I want to someone or you help me make cli option's help messages.

tomhoule · 2018-09-19T05:46:14Z

That looks really good :) I'll try to find the time so we can do the CLI help messages asap (probably this evening for me so in 10 hours or so).

tomhoule

Very nice! Let's not worry too much about the help messages for now, the ones that are there are good in my opinion and we can expand them in subsequent PRs.

tomhoule · 2018-09-20T05:30:43Z

graphql_client_cli/src/main.rs

+        /// Path to graphql schema file
+        #[structopt(parse(from_os_str))]
+        schema_path: PathBuf,
+        /// Name of struct


I have ideas on how we might change this in the future (because we can basically generate code for all operations), so I just opened issue #134. It will be a big refactoring and it needs to be discussed so let us merge this PR first :)

tomhoule · 2018-09-20T05:34:15Z

graphql_client_cli/src/main.rs

+        schema_path: PathBuf,
+        /// Name of struct
+        selected_operation: String,
+        /// Additional derives


nitpick: we could add a bit more info: "Additional derives that will be added to the generated structs and enums for the response and the variables"

tomhoule · 2018-09-20T05:34:31Z

graphql_client_cli/src/main.rs

+        /// --additional-derives='Serialize,PartialEq'
+        #[structopt(short = "a", long = "additional-derives")]
+        additional_derives: Option<String>,
+        /// allow, deny, or warn


nitpick: we could add something like "default: warn"

h-michael · 2018-09-20T06:00:17Z

@tomhoule
Thank you for giving me advice.
I fixed help messages and I've understood the issue(#134).

h-michael · 2018-09-20T06:02:25Z

After this PR is merged, I'll make a PR that update clid README.md.

tomhoule · 2018-09-20T06:06:36Z

Writing this here so one of us does it: we should also change the sentence about the CLI in the top-level README, because it is useful now.

h-michael · 2018-09-20T06:07:43Z

@tomhoule OK. Should I write down it before merge PR ?

tomhoule · 2018-09-20T06:08:23Z

No I think we can merge as soon as CI passes. It's not urgent.

tomhoule reviewed Sep 18, 2018

View reviewed changes

h-michael force-pushed the codegen branch 2 times, most recently from 98f60bd to ff932b2 Compare September 18, 2018 10:25

h-michael changed the title ~~[WIP] Enable generating graphql client code by cli~~ Enable generating graphql client code by cli Sep 18, 2018

h-michael force-pushed the codegen branch 3 times, most recently from b9758a1 to b335ac1 Compare September 18, 2018 11:58

h-michael added 2 commits September 19, 2018 16:12

Change GraphQLClientDeriveOptions fields

68d10bd

Enable generating graphql clent code by cli

ab629c3

h-michael force-pushed the codegen branch from b335ac1 to ab629c3 Compare September 19, 2018 07:14

tomhoule approved these changes Sep 20, 2018

View reviewed changes

Fix cli help messages

da185f2

tomhoule merged commit 42c185e into graphql-rust:master Sep 20, 2018

h-michael deleted the codegen branch September 20, 2018 06:32

Enable generating graphql client code by cli #122

Enable generating graphql client code by cli #122

Uh oh!

Conversation

h-michael commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h-michael commented Sep 17, 2018

Uh oh!

tomhoule left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-michael commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h-michael commented Sep 18, 2018

Uh oh!

h-michael commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h-michael commented Sep 19, 2018

Uh oh!

tomhoule commented Sep 19, 2018

Uh oh!

tomhoule left a comment

Choose a reason for hiding this comment

Uh oh!

tomhoule Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-michael commented Sep 20, 2018

Uh oh!

h-michael commented Sep 20, 2018

Uh oh!

tomhoule commented Sep 20, 2018

Uh oh!

h-michael commented Sep 20, 2018

Uh oh!

tomhoule commented Sep 20, 2018

Uh oh!

Uh oh!

h-michael commented Sep 17, 2018 •

edited

Loading

h-michael commented Sep 18, 2018 •

edited

Loading

h-michael commented Sep 19, 2018 •

edited

Loading

tomhoule Sep 20, 2018 •

edited

Loading