main
11 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Matúš Bafrnec
|
00f27609f0 |
fixed "for" directive documentation (#235)
This PR is addressing the behavior described in #151. The former version of `for` directive example usage ended in: ``` genqlient.graphql:<line>: for is only applicable to operations and arguments ``` I have: - [x] Written a clear PR title and description (above) - [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla) - [x] Added tests covering my changes, if applicable - [x] Included a link to the issue fixed, if applicable - [x] Included documentation, for new features - [x] Added an entry to the changelog Signed-off-by: Matúš Bafrnec <matus.bafrnec@gmail.com> |
||
|
Ben Kraft
|
67f2575cae |
Reject the use of both typename and bind (#172)
In #133, Craig added support for a new use of typename, where it applies to a scalar and means that genqlient should generate a named type, e.g. `# @genqlient(typename: "MyString")` on a node of type string will generate and use `type MyString string`. But this gets a bit confusing if you mix it with `bind`; should `typename: "MyString", bind: "int32"` generate `type MyString int32`, or should one override the other, or what? Of course in practice you're not likely to write that all in one place, but you could via a global binding, or a `for` directive, and in that case probably it was a mistake. In #138, we looked at making them work together correctly, but it added complexity and got even more confusing. So instead, here, we just ban it; we can always add it back if it proves useful. (Or, you can make the `typename` win over a global binding by locally unbinding it via `bind: "-"`.) This required changes in surprisingly many places; I already knew the directive-validation code was due for a refactor but that will happen some other day. The tests show that it works, in any case. Interestingly, this problem actually could have arisen for a struct binding already, before #133. But all the same reasons it's confusing seem to apply, so I just banned it there too. This is technically a breaking change although I doubt anyone will hit it. Test plan: make check |
||
|
Craig Silverstein
|
a52e55632f |
Allow creating aliases for builtin types, using typename. (#133)
## Summary:
This lets you write code like:
```
query x {
# @genqlient(typename: "MyString")
someStringField
}
```
and genqlient will do
```
typename MyString string
type x struct {
someStringField MyString
}
```
This was not difficult to implement, though it required introducing a
new identifier type. The main difficulty I had was weird test
failures, that it turns out was due to the tests putting a bunch of
fields on the same line, so that the genqlient directive on the
previous line applied to all of them, accidentally. This became a
problem when `typename` suddenly started being respected for builtin
types! I fixed it by just spreading out the queries a bit.
Fixes #130
## Test plan:
make check
Author: csilvers
Reviewers: dnerdy, StevenACoffman, benjaminjkraft
Required Reviewers:
Approved By: StevenACoffman
Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint
Pull Request URL: https://github.com/Khan/genqlient/pull/133
|
||
|
Ben Kraft
|
dd719deb4e |
Add a mechanism to specify options on input-type fields (#124)
## Summary: This has been a bit of a thorn since we started using genqlient in production: just as you might want to specify, say, `omitempty` on an argument, you might equally want to specify it on an input-type field. But there's no obvious syntax to do that, because the input-type field does not appear in the query (only the schema) so there's nowhere to put the `# @genqlient` directive. This commit, at last, fixes that problem, via a new option, `for`, which you use in an option applied to the entire operation (or fragment), and says, "actually, apply this directive to the given field, not the entire operation". (It's mainly useful for input types, but I allowed it for output types too; I could imagine it being convenient if you want to say you always use a certain type or type-name for a certain field.) It works basically like you expect: the inline options take precedence over `for` take precedence over query-global options. The implementation was fairly straightforward once I did a little refactoring, mostly in the directive-parsing and directive-merging (which are now combined, since merging is now a bit more complicated). With that in place, and extended to support `for`, we need only add the same wiring to input-fields that we have for other places you can put directives. I did not attempt to solve the issue I've now documented as #123, wherein conflicting options can lead to confusing behavior; the new `for` is a new and perhaps more attractive avenue to cause it but the issue remains the same and requires nontrivial refactoring (described in the issue) to solve. (The breakage isn't horrible for the most part; the option will just apply, or not apply, where you don't expect it to.) But while applying that logic, I noticed a problem, which is that we were inconsistently cascading operation-level options down to input-object fields. (I think this came out of the fact that initially I thought to cascade them, then realized that this could cause problems like #123 and intended to walk them back, but then accidentally only "fixed" it for `omitempty`. I guess until this change, operation-level options were rare enough, and input-field options messy enough, that no one noticed.) So in this commit I bring things back into consistency, by saying that they do cascade: with at least a sketch of a path forward to solve #123 via better validation, I think that's by far the clearest behavior. Issue: https://github.com/Khan/genqlient/issues/14 ## Test plan: make check Author: benjaminjkraft Reviewers: csilvers, StevenACoffman, benjaminjkraft, aberkan, dnerdy, jvoll, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: csilvers, StevenACoffman Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint Pull Request URL: https://github.com/Khan/genqlient/pull/124 |
||
|
Ben Kraft
|
c6d087c29b |
Add support for "flattening" fragment-spreads (#121)
## Summary:
One common use of fragment spreads is as the entirety of a field's
selection, e.g.
```graphql
query MyQuery {
myField {
...MyFragment
}
}
```
In this case, by default, genqlient generates a wrapper type
`MyQueryMyFieldMyType`, which just embeds `MyFragment`. This makes
sense if you later want to add more fields in addition to the fragment
spread. But if you don't -- and you did the fragment because you want
to share types, it's an extra layer of indirection. (Which becomes
especially onerous if `myField` has list type (`[MyType!]`), such that
it's not just an extra attribute-access to get to `MyFragment`.)
The new option `# @genqlient(flatten: true)` simplifies this situation:
if applied to `myField` is skips the wrapper type;
`MyQueryResponse.MyField` will simply have type `MyFragment` (or
`[]MyFragment`, or whatever). This should hopefully make the `typename`
option, which has more limitations, less necessary.
Note that in #30 the initial idea was to support this for fields as
well. This would require significant additional complexity in the
JSON-(un)marshaling code, and has proven less necessary, so I
implemented this option only for fragment-spreads for now. With that
restriction, it was shockingly simple; we have to hook into a bunch of
different places, but they're all quite simple, since the structure of
the Go types still matches the structure in GraphQL.
Issue: https://github.com/Khan/genqlient/issues/30
## Test plan:
make check
Author: benjaminjkraft
Reviewers: csilvers, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo, StevenACoffman
Required Reviewers:
Approved By: csilvers, dnerdy
Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint
Pull Request URL: https://github.com/Khan/genqlient/pull/121
|
||
|
Ben Kraft
|
6e2b1a5811 |
Fix broken behavior for several options on the same node (#105)
## Summary: Previously, we actually allowed you to put several genqlient directives on the same node, but the semantics were undocumented (and somewhat confusing, when it comes to `typename`). In order to support directives on input options, we're actually going to be encouraging this usage (see notes in #14), so it's time to fix it. To avoid confusion, I just had conflicting directives be an error, rather than defining which one "wins". The same applies to specifying the same option several times in one directive. I also fixed two small bugs: - `typename` on an operation would incorrectly cascade down to all input types in a query (causing conflicts). - directive parse errors had useless positions, now they're correct ## Test plan: make check Author: benjaminjkraft Reviewers: StevenACoffman, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman, dnerdy Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Lint Pull Request URL: https://github.com/Khan/genqlient/pull/105 |
||
|
Ben Kraft
|
8de55d352e |
Add support for binding with a custom marshal/unmarshal function (#104)
## Summary: This is useful if you want to bind to a type you don't control (or use for other things) but need different serialization than its default. This is a feature gqlgen has and we've found it very useful. For example, in webapp we want to bind `DateTime` to `time.Time`, but its default serialization is not compatible with Python, so currently we have to bind to a wrapper type and cast all over the place, which is exactly the sort of boilerplate genqlient is supposed to avoid. For unmarshaling, the implementation basically just follows the existing support for abstract types; instead of calling our own generated helper, we now call your specified function. This required some refactoring to abstract the handling of custom unmarshalers generally from abstract types specifically, and to wire in not only the unmarshaler-name but also the `generator` (in order to compute the right import alias). For marshaling, I had to implement all that stuff over again; it's mostly parallel to unmarshaling (and I made a few minor changes to unmarshaling to make the two more parallel). Luckily, after #103 I at least only had to do it once, rather than implementing the same functionality for arguments and for input-type fields. It was still quite a bit of code; I didn't try to be quite as completionist about the tests as with unmarshal but still had to add a few. Issue: https://github.com/Khan/genqlient/issues/38 ## Test plan: make check Author: benjaminjkraft Reviewers: StevenACoffman, dnerdy, benjaminjkraft, aberkan, jvoll, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman, dnerdy Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint Pull Request URL: https://github.com/Khan/genqlient/pull/104 |
||
|
Ben Kraft
|
5995653583 |
Refactor argument-handling to use a struct (#103)
## Summary: In this commit I refactor the argument-generation logic to move most of the code out of the template and into the type-generator. This logic predates #51, and I didn't think to update it there, but I think it benefits from similar treatment, for similar reasons. Specifically, the main change is to treat variables as another struct type we can generate, rather than handling them inline as a `map[string]interface{}`. Users still pass them the same way, but instead of putting them into a `map[string]interface{}` and JSONifying that, we generate a struct and put them there. This turns out to simplify things quite a lot, because we already have a lot of code to generate types. Notably, the omitempty code goes from a dozen lines to basically two, and fixes a bug (#43) in the process, because now that we have a struct, `json.Marshal` will do our work for us! (And, once we have syntax for it (#14), we'll be able to handle field-level omitempty basically for free.) More importantly, it will simplify custom marshalers (#38, forthcoming) significantly, since we do all that logic at the containing-struct level, but will need to apply it to arguments. It does require two breaking changes: 1. For folks implementing the `graphql.Client` API (rather than just calling `NewClient`): we now pass them variables as an `interface{}` rather than a `map[string]interface{}`. For most callers, including Khan/webapp, this is basically a one-line change to the signature of their `MakeRequest`, and it should be a lot more future-proof. 2. genqlient's handling of the `omitempty` option has changed to match that of `encoding/json`, in particular it now never considers structs "empty". The difference was never intentional (I just didn't realize that behavior of `encoding/json`); arguably our behavior was more useful but I think that's outweighed by the value of consistency with `encoding/json` as well as the simpler and more correct implementation (fixing #43 is actually quite nontrivial otherwise). Once we have custom unmarshaler support (#38), users will be able to map a zero value to JSON null if they wish, which is mostly if not entirely equivalent for GraphQL's purposes. Issue: https://github.com/Khan/genqlient/issues/38 Issue: https://github.com/Khan/genqlient/issues/43 ## Test plan: make check Author: benjaminjkraft Reviewers: StevenACoffman, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman, dnerdy Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14) Pull Request URL: https://github.com/Khan/genqlient/pull/103 |
||
|
Ben Kraft
|
fcae8dd1d7 |
Add support for specifying type-names, and conflict-detection (#94)
## Summary: In this commit I add two related features to genqlient: conflict-detection to avoid generating two distinct types with the same name, and an option to specify the type-name genqlient should use for some type. The conflict-detection was pretty simple once I realized I had already written all the code to do it in #70. There was a bunch of wiring, since we now need to keep track of the GraphQL type/selection-set that each type corresponds to, but it was pretty straightforward. This allows us to: - detect and reject if you have really sneaky type-names (there are some examples documented in `names.go`) - more clearly crash if genqlient accidentally generates two conflicting types, and - avoid stack-overflow when handing recursive (input) types (although sadly the poor support for options on input types (#14) makes them difficult to use in many cases; you really need to be able to set `pointer: true`) And with that all set up, the type-naming was also easy! (It doesn't have to get into the core of the type-generator, just plug in where we choose names. The desire for conflict detection was the main reason I hadn't set it up already.) Note that the existing limitation of #70 that the fields have to be in exactly the same order remains (and is now documented as #93); it's not deeply hard to fix but it's surprisingly much work. Issue: https://github.com/Khan/genqlient/issues/60 Issue: https://github.com/Khan/genqlient/issues/12 ## Test plan: make check Author: benjaminjkraft Reviewers: StevenACoffman, jvoll, benjaminjkraft, aberkan, csilvers, dnerdy, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman, jvoll Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint Pull Request URL: https://github.com/Khan/genqlient/pull/94 |
||
|
Ben Kraft
|
5211442843 |
Add a new option to treat an interface like an object (#97)
## Summary: The basic idea here is if you only request interface fields (no fragments) you may not care about the concrete type, and so we could just generate a struct as if it were an object. I don't think it's a good idea to do that by default, because then if you later add a fragment all your code totally changes, but it's quite reasonable as an option! Most of the code involved is just wiring and validation; the core implementation is literally just: treat it like an object. Issue: https://github.com/Khan/genqlient/issues/85 ## Test plan: make check Author: benjaminjkraft Reviewers: csilvers, StevenACoffman, benjaminjkraft, aberkan, dnerdy, jvoll, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Lint Pull Request URL: https://github.com/Khan/genqlient/pull/97 |
||
|
Ben Kraft
|
2eba9a2c30 |
Reorganize documentation to make room to grow (#84)
## Summary: In this commit I reorganize much of our documentation into a new `docs` directory, where there will hopefully be more room to grow and to organize things in a user-friendly way. There's almost no net-new documentation, although of course it's a great time to review it anyway. In particular: - I moved the documentation for the `genqlient.yaml` config file into an example file instead of GoDoc (which now just points to the example file); I think this will be a lot clearer for casual users. - I moved the documentation for the `@genqlient` directive out of GoDoc and into a GraphQL schema file (since while it's a comment it's all real syntax), likewise, and made the `GenqlientDirective` type private (since there's now nothing useful to do with it). - I moved `DESIGN.md` and the logo into `docs/` (just to keep the toplevel a bit cleaner), and separated the Contributing section of the README into `docs/CONTRIBUTING.md` (which github will automatically link on various issue and PR pages). This leaves it so that: - README.md is the only documentation at the toplevel (and will become just the high-level introduction as I add more user docs to `docs/`) - GoDoc is only documentation for if you want to call genqlient programmatically (which is fairly limited as the API surface is quite small: it's now just Main, Generate, and Config, plus a constructor, a single method, and a bunch of fields on the latter) In future commits, I'll add some more new documentation to the `docs` directory. Issue: https://github.com/Khan/genqlient/issues/26 ## Test plan: make check (and read the docs) Author: benjaminjkraft Reviewers: jvoll, benjaminjkraft, aberkan, dnerdy, MiguelCastillo, mahtabsabet Required Reviewers: Approved By: jvoll Checks: ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint Pull Request URL: https://github.com/Khan/genqlient/pull/84 |