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

Default to None in client for optional arguments #254

Merged

Conversation

kitlangton
Copy link
Contributor

@kitlangton kitlangton commented Feb 29, 2020

Closes #251

This will add None as the default value for any optional argument. I know that you (@ghostdogpr) suggested that I might want to add defaults only when the optionals come at the end. I was thinking it would still be useful to have this feature even if the optional arguments weren't trailing, as I could switch to using named parameters. But I'll happily change it if you'd prefer it the other way :)

@ghostdogpr
Copy link
Owner

Looks good! Can you also update the generated code in the examples project? There’s at least one case of optional argument.

@kitlangton
Copy link
Contributor Author

Will do!

@kitlangton
Copy link
Contributor Author

@ghostdogpr Is the client generated from a schema file? I can't seem to find any schema files in the repo. How should I go about regenerating the example code? I'd be happy to document the process somewhere as well.

@ghostdogpr
Copy link
Owner

Right! I didn’t upload it 😄 It’s obtained by calling .render on any of the server example projects (I used the http4s one but I think they’re all same).

@ghostdogpr
Copy link
Owner

Here it is:

union Role = Captain | Engineer | Mechanic | Pilot

type Engineer {
    shipName: String!
}

type Subscriptions {
    characterDeleted: String!
}

type Mutations {
    deleteCharacter(name: String!): Boolean!
}

type Queries {
    characters(origin: Origin): [Character!]!
    character(name: String!): Character
}

enum Origin {
    BELT
    EARTH
    MARS
}

type Character {
    name: String!
    nicknames: [String!]!
    origin: Origin!
    role: Role
}

type Pilot {
    shipName: String!
}

type Mechanic {
    shipName: String!
}

type Captain {
    shipName: String!
}

schema {
    query: Queries
    mutation: Mutations
    subscription: Subscriptions
}

@kitlangton
Copy link
Contributor Author

I'm having trouble using the plugin from within the caliban project. It doesn't register the calibanGenClient command. What commands work for you?

@kitlangton
Copy link
Contributor Author

Also, I was thinking, along the lines of default arguments: What would you think of adding a default of Empty list for List arguments?

@ghostdogpr
Copy link
Owner

I'm having trouble using the plugin from within the caliban project. It doesn't register the calibanGenClient command. What commands work for you?

Did you 1) publish the plugin locally 2) add the dependency to plugins.sbt 3) enable the plugin in the related project 4) reload sbt ? It works if I do that.

Also, I was thinking, along the lines of default arguments: What would you think of adding a default of Empty list for List arguments?

Sounds good, let's add = Nil.

@kitlangton
Copy link
Contributor Author

kitlangton commented Mar 2, 2020

After running the plugin, it looks like the Role union is no longer nested within a Role object in the generated Client. I don't think my code made this change, as the same thing happens if I generate the schema with the 0.6.0 published plugin. Just wanted to flag that :)

-  object Role {
-    type Captain
-    object Captain {
-      def shipName: SelectionBuilder[Captain, String] = Field("shipName", Scalar())
-    }
-    type Engineer
-    object Engineer {
-      def shipName: SelectionBuilder[Engineer, String] = Field("shipName", Scalar())
-    }
-    type Mechanic
-    object Mechanic {
-      def shipName: SelectionBuilder[Mechanic, String] = Field("shipName", Scalar())
-    }
-    type Pilot
-    object Pilot {
-      def shipName: SelectionBuilder[Pilot, String] = Field("shipName", Scalar())
-    }
+  type Engineer
+  object Engineer {
+    def shipName: SelectionBuilder[Engineer, String] = Field("shipName", Scalar())
   }

@ghostdogpr
Copy link
Owner

@kitlangton Right, I changed it at some point and forgot to regenerate the example 😄

@kitlangton
Copy link
Contributor Author

kitlangton commented Mar 2, 2020

Whew. Glad I'm not crazy :P. Why was that change made by the way?
One downside is that the members get separated within the new generated client (Engineer is above Character now):

  type Engineer
  object Engineer {
    def shipName: SelectionBuilder[Engineer, String] = Field("shipName", Scalar())
  }

  type Character
  object Character {
    def name: SelectionBuilder[Character, String]            = Field("name", Scalar())
    def nicknames: SelectionBuilder[Character, List[String]] = Field("nicknames", ListOf(Scalar()))
    def origin: SelectionBuilder[Character, Origin]          = Field("origin", Scalar())
    def role[A](
      onCaptain: SelectionBuilder[Captain, A],
      onEngineer: SelectionBuilder[Engineer, A],
      onMechanic: SelectionBuilder[Mechanic, A],
      onPilot: SelectionBuilder[Pilot, A]
    ): SelectionBuilder[Character, Option[A]] =
      Field(
        "role",
        OptionOf(
          ChoiceOf(
            Map(
              "Captain"  -> Obj(onCaptain),
              "Engineer" -> Obj(onEngineer),
              "Mechanic" -> Obj(onMechanic),
              "Pilot"    -> Obj(onPilot)
            )
          )
        )
      )
  }

  type Pilot
  object Pilot {
    def shipName: SelectionBuilder[Pilot, String] = Field("shipName", Scalar())
  }

  type Mechanic
  object Mechanic {
    def shipName: SelectionBuilder[Mechanic, String] = Field("shipName", Scalar())
  }

  type Captain
  object Captain {
    def shipName: SelectionBuilder[Captain, String] = Field("shipName", Scalar())
  }

@ghostdogpr
Copy link
Owner

That's because a type can be a member of multiple unions/interfaces, it doesn't actually belong to it. I found out when testing with GitHub API. Some types were created multiple times under each union/interface they belonged to.

@kitlangton
Copy link
Contributor Author

Gotcha! Makes sense :)

@kitlangton kitlangton force-pushed the add-defaults-for-nullable-arguments branch from f3f3898 to 633da11 Compare March 2, 2020 01:59
@kitlangton
Copy link
Contributor Author

@ghostdogpr Alrighty :) Added Nil defaults and regenerated the Client.

@kitlangton
Copy link
Contributor Author

Doh. I need to format

@kitlangton kitlangton force-pushed the add-defaults-for-nullable-arguments branch from 633da11 to f78d5e8 Compare March 2, 2020 02:03
@kitlangton
Copy link
Contributor Author

Good to go 👍

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

Awesome! Just a tiny remark and we can merge.

case list =>
s"(${list.map(arg => s"${safeName(arg.name)}: ${writeType(arg.ofType)}").mkString(", ")})"
case Nil => ""
case list => s"(${writeArgumentFields(list)})"
Copy link
Owner

Choose a reason for hiding this comment

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

You can remove the quotes now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to add parens in this case, I believe. Is that what you mean?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah sorry i was not seeing properly. All good!

@ghostdogpr ghostdogpr merged commit 9fe7294 into ghostdogpr:master Mar 2, 2020
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.

Generate default values of None for optional arguments
2 participants