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

Omitempty-style handling for array elements #44

Open
benjaminjkraft opened this issue May 3, 2021 · 1 comment
Open

Omitempty-style handling for array elements #44

benjaminjkraft opened this issue May 3, 2021 · 1 comment
Labels
enhancement New feature or request needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing needs use cases Feature we can add if people find use cases that need it (comment if that's you)

Comments

@benjaminjkraft
Copy link
Collaborator

Suppose you have an argument that's a [String]! and you want to pass ["a", null, "c"]. Right now you pretty much have to do pointer: true, then pass []*string{&"a", null, &"c"} (except of course that's not valid syntax, the real syntax is uglier until golang/go#45624). But we should be able to just let you apply the omitempty to the elements, instead of or in addition to to the whole array, so that you can pass []string{"a", "", "c"} and it does the thing you mean. It may need a name since it's really "translate empty elements to null", rather than omitting them.

See also #16 and #14.

benjaminjkraft added a commit that referenced this issue Aug 27, 2021
Summary:
We had this setting called "scalars", which said: bind this GraphQL type
to this Go type, rather than the one you would normally use.  It's
called that because it's most useful for custom scalars, where "the one
you would normally use" is "error: unknown scalar".  But nothing ever
stopped you from using it for a non-scalar type.  I was planning on
removing this functionality, because it's sort of a rough edge, but a
discussion with Craig found some good use cases, so instead, in this
commit, I document it better and add some slightly nicer ways to specify
it.

Specifically, here are a few potential non-scalar use cases:
- bind a GraphQL enum to a nonstandard type (or even `string`)
- bind an input type to some type that has exactly the fields you want;
  this acts as a sort of workaround for issues #14 and #44
- bind an object type to your own struct, so as to add methods to it
  (this is the use case Craig raised)
- bind an object type to your own struct, so as to share it between
  multiple queries (I believe named fragments will address this case
  better, but it doesn't hurt to have options)
- bind a GraphQL list type to a non-slice type in Go (presumably one
  with an UnmarshalJSON method), or any other different structure
The latter three cases still have the sharp edge I was originally
worried about, which is that nothing guarantees that the fields you
request in the query are the ones the type expects to get.  But I think
it's worth having the option, with appropriate disclaimers.

The main change to help support that better is that you can now specify
the type inline in the query, as an alternative to specifying it in the
config file; this means you might map a given object to a given struct,
but only in some cases, and when you do you have a chance to look at the
list of fields you're requesting.

Additionally, I renamed the config field from "scalars" to "bindings"
(but mentioned it in a few places where you might go looking for how to
map scalars, most importantly the error message you get for an unknown
(custom) scalar).  While I was making a breaking change, I also changed
it to be a `map[string]<struct>` instead of a `map[string]string`,
because I expect to add more fields soon, e.g. to handle issue #38.

Finally, since the feature is now intended/documented, I added some
tests, although it's honestly quite simple on the genqlient side.

Test Plan: make tesc

Reviewers: csilvers, marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 27, 2021
…pe (#69)

## Summary:
We had this setting called "scalars", which said: bind this GraphQL type
to this Go type, rather than the one you would normally use.  It's
called that because it's most useful for custom scalars, where "the one
you would normally use" is "error: unknown scalar".  But nothing ever
stopped you from using it for a non-scalar type.  I was planning on
removing this functionality, because it's sort of a rough edge, but a
discussion with Craig found some good use cases, so instead, in this
commit, I document it better and add some slightly nicer ways to specify
it.

Specifically, here are a few potential non-scalar use cases:
- bind a GraphQL enum to a nonstandard type (or even `string`)
- bind an input type to some type that has exactly the fields you want;
  this acts as a sort of workaround for issues #14 and #44
- bind an object type to your own struct, so as to add methods to it
  (this is the use case Craig raised)
- bind an object type to your own struct, so as to share it between
  multiple queries (I believe named fragments will address this case
  better, but it doesn't hurt to have options)
- bind a GraphQL list type to a non-slice type in Go (presumably one
  with an UnmarshalJSON method), or any other different structure
The latter three cases still have the sharp edge I was originally
worried about, which is that nothing guarantees that the fields you
request in the query are the ones the type expects to get.  But I think
it's worth having the option, with appropriate disclaimers.

The main change to help support that better is that you can now specify
the type inline in the query, as an alternative to specifying it in the
config file; this means you might map a given object to a given struct,
but only in some cases, and when you do you have a chance to look at the
list of fields you're requesting.

Additionally, I renamed the config field from "scalars" to "bindings"
(but mentioned it in a few places where you might go looking for how to
map scalars, most importantly the error message you get for an unknown
(custom) scalar).  While I was making a breaking change, I also changed
it to be a `map[string]<struct>` instead of a `map[string]string`,
because I expect to add more fields soon, e.g. to handle issue #38.

Finally, since the feature is now intended/documented, I added some
tests, although it's honestly quite simple on the genqlient side.

## Test plan:
make tesc


Author: benjaminjkraft

Reviewers: csilvers, aberkan, dnerdy, MiguelCastillo

Required Reviewers: 

Approved by: csilvers

Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ⌛ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ⌛ Lint

Pull request URL: #69
@benjaminjkraft
Copy link
Collaborator Author

benjaminjkraft commented Sep 3, 2021

As I mention in the above-linked commit, the new bindings option is also a sort of workaround to this.

@benjaminjkraft benjaminjkraft added the needs use cases Feature we can add if people find use cases that need it (comment if that's you) label Sep 3, 2021
@benjaminjkraft benjaminjkraft added enhancement New feature or request needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing labels Sep 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing needs use cases Feature we can add if people find use cases that need it (comment if that's you)
Projects
None yet
Development

No branches or pull requests

1 participant