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

Go type of variables object. #4

Closed
dmitshur opened this issue Jun 20, 2017 · 1 comment
Closed

Go type of variables object. #4

dmitshur opened this issue Jun 20, 2017 · 1 comment

Comments

@dmitshur
Copy link
Member

dmitshur commented Jun 20, 2017

The variables object, which is optionally present as part of queries and mutations, must be valid JSON object.

By encoding/json rules, we can use a Go struct or a Go map type, which will then get JSON encoded into a JSON object.

During initial development, I started with the following signature for Query:

func (c *Client) Query(ctx context.Context, q interface{}, variables interface{}) error

And usage looked like this:

variables := struct {
	RepositoryOwner githubql.String
	RepositoryName  githubql.String
	IssueNumber     githubql.Int
}{githubql.String(repo.Owner), githubql.String(repo.Repo), githubql.Int(id)}
err = client.Query(ctx, &v, variables)

However, having to define a new struct for each variables object seemed less optimal than using a map. Mostly because the Go syntax has you repeating the fields and the field values twice, as well as doing type conversions.

Compare to the syntax when using a map:

func (c *Client) Query(ctx context.Context, q interface{}, variables map[string]interface{}) error
variables := map[string]interface{}{
	"RepositoryOwner": githubql.String(repo.Owner),
	"RepositoryName":  githubql.String(repo.Repo),
	"IssueNumber":     githubql.Int(id),
}
err = client.Query(ctx, &v, variables)

That seems easier to write and read, so that was the API decision I went with for the initial commit.

@dmitshur
Copy link
Member Author

This decision has been made in 78a7455.

Closing this issue. It exists here for posterity, so the decision and the reasoning behind it can be seen, and if needed, revisited in the future.

dmitshur added a commit that referenced this issue Jun 20, 2017
This is more idiomatic GraphQL style. There used to be a technical
reason that they had to begin with an uppercase letter before #4,
but that's no longer the case.

Since there's no reason to deviate from GraphQL style here, the plan
is to follow it more closely for now. That is, until there's a good
reason to deviate from it.

This change is really just a documentation/example change. Users can
still name variables whatever they want.

Resolves #5.
dmitshur added a commit that referenced this issue Jun 20, 2017
This is more idiomatic GraphQL style. There used to be a technical
reason that they had to begin with an uppercase letter before #4,
but that's no longer the case.

Since there's no reason to deviate from GraphQL style here, the plan
is to follow it more closely for now. That is, until there's a good
reason to deviate from it.

This change is really just a documentation/example change. Users can
still name variables whatever they want.

Resolves #5.
@dmitshur dmitshur changed the title Variables object Go type. Go type of variables object. Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

1 participant