Lint is failing with some inscrutable panic (on a commit with no code
changes). Let's try bumping the version in case they fixed it.
Additionally, the new version's github action uses Go 1.19, which means
it pulls in gofmt updates to match the new [doc-comment formatting rules][1].
So I added Go 1.19 to our list of versions to test (fixes#216)
and updated some of our doc-comments to format better in the
new world (mostly using the new link syntax).
[1]: https://go.dev/doc/comment
Test plan: make lint
It feels like just yesterday, but it's been over four months since our
last release! So it's as good a time as any; while there are quite a few
changes they're individually mostly small. As usual, this updates the
changelog, and I'll tag it with the release once it lands.
Test plan: no relevant bug reports lately
While writing tests at some point I came across an invalid query that
gqlparser doesn't catch, and which causes a panic for us. Now we
validate for it and return a nice error instead of panicing.
Fixes#176.
Test plan: make check
GraphQL is pretty restrictive about its identifiers, so for the most
part we can and do safely use GraphQL identifiers in the Go we generate
with attention only to conflicts with other such identifiers. But we do
need to check one thing, which is that the identifier isn't a Go
keyword. (If it is, the generated code will almost certainly fail to
compile, but often with a confusing error message.) In this commit I add
such checks.
The most likely place to run into trouble here is argument names, which
are often one word and are used as-is. Operation names, if unexported,
can also be keywords, although in practice they're usually multiword.
Field names are always exported, thus safe. Generated type names are
always prefixed, camel-cased, with the operation name, so they always
contain an uppercase letter (even if the operation name is lowercase),
but type-names specified by `typename` may collide, so we check those.
In theory we could check type-names specified by `bind`, but these must
be defined by the user, so their code will already fail to compile, so I
didn't bother. I think that's all the places to consider, although it's
hard to be sure. In summary, we check argument names, operation names,
and user-specified type names.
Test plan: make check
## 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
## Summary:
It's nice to be clear here because it may be your first interaction with
genqlient!
## Test plan:
make check
Author: benjaminjkraft
Reviewers: StevenACoffman, benjaminjkraft, dnerdy
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/117
## 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
## Summary:
genqlient has some code (`imports.go`) dedicated to tracking which
imports we need and avoiding conflicts, as well as converting a
(restricted) Go expression like `map[string]github.com/me/mypkg.MyType`
to an import (`github.com/me/mypkg`) and a type-reference
(`map[string]mypkg.MyType`) to be used in the context of that import,
and at least making some attempt to track conflicts. (Right now the
conflict-avoidance is not very smart, and not very well tested, but it
comes up rarely anyway.) Sadly, that code was a bit cumbersome to use,
because you had to first register the imports (typically from
`convert.go`), then use them (often from the template).
In this commit I refactor the order we write things in order to allow a
significant simplification of how we import; in particular we no longer
have to guess in advance what imports which template will need; it can
just do `{{ref <expr>}}` as before, and it just works. To do this, I:
- changed the importer to have only one API, which adds the import if
needed, and returns the reference either way
- added a check that we don't add imports after they're written
- reorganized the toplevel templates a bit to make sure that check never
fires; we now generate all the types and operations, then write the
imports and glue it all together
This removes a bunch of silly code, and should simplify the process of
adding custom (un)marshalers (#38).
While I was at it, I put the documentation of what expressions we
support in a more visible place, and added a type-assertion that your
custom context type implements context.Context (if applicable).
## 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/101
## 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
## 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
## Summary:
Before open-sourcing, we want to make sure that (a) GoDoc looks
reasonable, and (b) everything in the API is something we want to commit
to. In this commit, I do some miscellaneous cleanup on both fronts;
this does involve a few breaking changes to the programmatic API (better
now than once it has users). In future commits, I'll likely move the
documentation for `genqlient.yaml` and `@genqlient` to clearer places,
and make `GenqlientDirective` private, such that GoDoc is really only
for programmatic users.
Fixes#25.
Issue: https://github.com/Khan/genqlient/issues/25
## Test plan:
make check
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, jvoll, aberkan, MiguelCastillo, mahtabsabet
Required Reviewers:
Approved By: dnerdy, 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/82
## Summary:
Steve pointed out (#73) that having genqlient with no arguments silently
use a default config file was a bit confusing, and changed it to use
`genqlient.yaml` by default (#74). Mark pointed out (#76) that this
makes it a bit less convenient when you're starting from scratch; you
have to go create a config file. In this commit I add a new init flag
that creates you a config file before using it.
Originally the suggestion was to use subcommands, e.g. we'd have
`genqlient init` and `genqlient generate` and so on. But I couldn't
think of anything else we might want subcommands for in the future, and
it felt a little silly to make you type `generate` each time. So
instead, I made it a flag, which has the nice property that you can do
`genqlient --init` and it will generate and then use a config file. (I
mean, maybe it will immediately crash because you don't have a schema,
but hopefully that's still a useful clue as to what to do next!) The
implmentation was fairly trivial.
Since we now have a nice way to generate a default config, I removed the
default values for most of the options; I've always felt they were
probably more confusing than helpful. (And indeed, all the users I know
of (Khan/webapp, and the much smaller project Steve was working on, are
setting those options explicitly.) This required a slight change to
the syntax to say "don't use context", which is probably also net clearer.
I decided this is also a good time to pull in a proper CLI parser (#31);
see ADR-504 for more on that choice. This also adds some nice help
messages!
Fixes#76, #31.
Issue: https://github.com/Khan/genqlient/issues/76
## Test plan:
```
go run .
go run . --init
go run . --init example/genqlient.yaml # refuses to clobber
go run . --init example/newgenqlient.yaml
```
Author: benjaminjkraft
Reviewers: dnerdy, aberkan, MiguelCastillo, StevenACoffman
Required Reviewers:
Approved By: 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/81
## Summary:
In previous commits I added support to genqlient for interfaces,
inline fragments, and, most recently, named fragments of concrete
(object) type. This leaves only named fragments of interface type!
Like other named fragments, these are useful for code-sharing,
especially if you want some code that can handle the same fields of
several different types.
As seems to be inevitable with genqlient, this was mostly pretty
straightforward, although there turned out to be surprisingly many
places we needed to add some handling; almost anywhere that touches
interfaces *or* named fragments needed some updates. But it's all
hopefully fairly clear code.
As a part of this change I made three semi-related improvements:
1. I refactored the handling of descriptions (i.e. GoDoc), because it
was getting more and more confusing and duplicative. I'm still not
sure how much of it it makes sense to inline vs. separate, but I
think this is better than it was. This resulted in some minor
changes to descriptions, generally in the direction of making things
more consistent.
2. I bumped the minimum Go version to 1.14 so we can guarantee support
for duplicate interface methods. These are useful for
abstract-in-absstract spreads; we generate an interface for the
fragment, and (if the fragment-type implements the scope-type) we
embed it into the interface we generate for its spread-context, and
if the two have a duplicated field we thus duplicate the method. It
wouldn't be impossible to support this on 1.13 (maybe just by
omitting said embed) but it didn't seem worth it. This also removes
a few special-cases in tests.
3. I added a bunch of code to better format syntax errors in the
generated code (which we see from `gofmt`). This is mostly just an
internal improvement; I wrote it because I got annoyed while hunting
down a few such errors..
Fixes, at last, #8.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make check
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo
Required Reviewers:
Approved By: dnerdy
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/79
## Summary:
In previous commits I added support to genqlient for interfaces and
inline fragments. This means the only query structures that remain are
named fragments and their spreads, e.g.
```
fragment MyFragment on MyType { myField }
query MyQuery { getMyType { ...MyFragment } }
```
Other than mere completionism, these are potentially useful for code
sharing: you can spread the same fragment multiple places; and then
genqlient can notice that and generate the same type for each. (They
can even be shared between different queries in the same package.)
In this commit I add support for named fragments of concrete
(object/struct, not interface) type, spread into either concrete or
abstract scope. For genqlient's purposes, these are a new "root"
type-name, just like each operation, and are then embedded into the
appropriate struct. (Using embeds allows their fields to be referenced
as fields of the containing type, if convenient. Further design
considerations are discussed in DESIGN.md.)
This requires new code in two main places (plus miscellaneous glue),
both nontrivial but neither particularly complex:
- We need to actually traverse both structures and generate the types
(in `convert.go`).
- We need to decide which fragments from this package to send to the
server, both for good hyigene and because GraphQL requires we send
only ones this query uses (in `generate.go`).
- We need a little new wiring for options -- because fragments can be
shared between queries they get their own toplevel options, rather
than inheriting the query's options.
Finally, this required slightly subtler changes to how we do
unmarshaling (in `types.go` and `unmarshal.go.tmpl`). Basically,
because embedded fields' methods, including `UnmarshalJSON`, get
promoted to the parent type, and because the JSON library ignores their
fields when shadowed by those of the parent type, we need a little bit
of special logic in each such parent type to do its own unmarshal and
then delegate to each embed. This is similar (and much simpler) to
what we did for interfaces, although it required some changes to the
"method-hiding" trick (used for both). It's only really necessary in
certain specific cases (namely when an embedded type has an
`UnmarshalJSON` method or a field with the same name as the embedder),
but it's easier to just generate it always. This is all described in
more detail inline.
This does not support fragments of abstract type, which have their own
complexities. I'll address those, which are now the only remaining
piece of #8, in a future commit.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make check
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo
Required Reviewers:
Approved By: dnerdy
Checks: ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ 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: https://github.com/Khan/genqlient/pull/75
## Summary:
ContextType is in use at Khan as a part of our ka-context system; it
basically just lets you configure the type to pass as the `ctx` argument
to genqlient helpers (or say to omit such an argument). ClientGetter I
wrote thinking we might use it; then we didn't (because we have a few
different clients we may use) but it's not much code and may be helpful
to others. In this commit I clean up, document, and add tests for both
options.
The cleanup is mainly for ClientGetter, which was kind of broken before
because it was a Go snippet but couldn't specify imports. I was
thinking maybe you want to be able to write `ctx.Something()`, but I
just don't see how to make it work, so I made it a function of context,
which is probably the better idea anyway.
Additionally, I improved the documentation for both, and added tests for
those and several other config options that weren't completely tested.
Fixes#5.
Issue: https://github.com/Khan/genqlient/issues/5
## Test plan:
make check
Author: benjaminjkraft
Reviewers: dnerdy, aberkan, MiguelCastillo
Required Reviewers:
Approved By: dnerdy
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: https://github.com/Khan/genqlient/pull/77
## Summary:
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be `MyFieldMyType`, not
`MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly
wrong. But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.) In this commit, in
any case, I fix it.
To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer. In the past it was kind of
spread out, and each caller would have to pass the right name into
`convertDefinition`, which go quite unwieldy. Now, the case that really
wanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover. (I even had some fun using
a linked list to implement the prefix-stack!)
This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up. As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make tesc
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo
Required Reviewers:
Approved by: dnerdy
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: https://github.com/Khan/genqlient/pull/71
## Summary:
In #52, I added support for interface types, but with the simplifying
restriction (among others) that the user must request the field
`__typename`. In this commit, I remove this restriction.
The basic idea is simple: we preprocess the query to add `__typename`.
The implementation isn't much more complicated! Although it required
some new wiring in a few places.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make check
Author: benjaminjkraft
Reviewers: dnerdy, aberkan, MiguelCastillo
Required Reviewers:
Approved by: dnerdy
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: https://github.com/Khan/genqlient/pull/56
## Summary:
In this commit I begin the journey to add the long-awaited support for
interfaces (part of #8). Well, it's not the beginning: I already had
some half-written broken code around. But it's the first fully
functional support, and especially, the first *tested* support; it's
probably best to review the nontrivially-changed code as if it were new.
Conceptually, the code so far is pretty simple: we generate an interface
type, and the implementations. (That code is in fact mostly unchanged.)
The complexity comes in because encoding/json doesn't know how to
unmarshal that. So we have to add an UnmarshalJSON method, which
actually has to be on the types with interface-type fields, that knows
how. I factored it into two methods, such that that UnmarshalJSON
method is just glue, and then there's a separate function, corresponding
to each interface-type, that actually does all the work. (If only one
could just write it as an actual method!) The method uses the same
trick suggested to me by a few others in another context to deserialize
all but one field, then handle that field specially, which is discussed
in the code.
This still has some limitations, which will be lifted in future commits:
- it doesn't allow for list-of-interface fields
- it requires that you manually ask for `__typename`
- it doesn't support fragments, i.e. you can only query for interface
fields, not concrete-type-specific ones
But it works, even in integration tests, which is progress!
As a part of this, I added a proper config option for the "allow broken
features" flag, since I need to be able to set it from the integration
tests which are in a separate package (and actually shell out via `go
generate`). I also renamed what was to be the first case
(InterfaceNoFragments), and replaced it with a further-simplified
version (avoiding list-of-interface fields.
[1] https://github.com/benjaminjkraft/notes/blob/master/go-json-interfaces.md
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make tesc
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, aberkan, csilvers, MiguelCastillo
Required Reviewers:
Approved by: dnerdy
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: https://github.com/Khan/genqlient/pull/52
## Summary:
I've felt for a while that types.go is way too confusing, and as I
started to implement some of the more complex cases of generating
interface-types, the cracks were really starting to show. Luckily, I
also finally realized how to fix it: we need to separate the process of
traversing the GraphQL operation and schema to decide what types to
generate from the process of actually generating those types. This
requires an extra set of intermediate data structures, but I think it
makes things quite a lot easier to understand -- and, importantly, it
means that the code-generation doesn't need to go in the order we
traverse the query/schema.
So in this commit, I did that huge refactor. It's probably best to just
review types.go and traverse.go as if they were new; the old code was
quite hard to understand and the new code will hopefully make a lot more
sense. (And to that end, review comments about what could be organized
better or needs more documentation are very much in order, even for code
that is mostly unchanged.)
This does introduce one bug, sort of, which is that rather than
generating broken code for list-of-interface fields, we generate no code
at all. (A TODO in unmarshal.go describes why.) I'll fix this when I
add support for those fields. (It's all behind the AllowBrokenFeatures
flag, anyway.) Otherwise, the only changes to generated code are that a
few methods are ordered differently, because we now generate the
implements-interface methods with the interface, rather than the
implementations, as it's much simpler that way. (In GraphQL, unlike Go,
we know the list of all possible implementations of each interface, so
this is possible.)
## Test plan:
golangci-lint run ./... && go test ./...
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, aberkan, csilvers, MiguelCastillo
Required Reviewers:
Approved by: dnerdy
Checks: ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint
Pull request URL: https://github.com/Khan/genqlient/pull/51
## Summary:
It would be nice to have some linting beyond `go vet`! Now we do. I
started by copying the config from Khan/webapp. I did remove a couple
of staticcheck checks that I didn't feel were useful. (Note also that
exportloopref is the replacement for scopelint in newer golangci-lint.)
Included are all the needed lint fixes; most are stylistic but the
changes in the example are a (minor) bugfix.
Fixes#22.
Issue: https://github.com/Khan/genqlient/issues/22
## Test plan:
make check
Author: benjaminjkraft
Reviewers: aberkan, dnerdy, benjaminjkraft, csilvers, MiguelCastillo
Required Reviewers:
Approved by: aberkan, dnerdy
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: https://github.com/Khan/genqlient/pull/49
Ben Kraft