A few tweaks I noticed after doing this release:
- I forgot to add a summary line to this release; and forgot to mention
this in the release documentation.
- Steve suggested we create GitHub releases, so I added his instructions
for doing that.
- An older documentation link was wrong.
It's been quite a while! Let's cut a release. I also added some docs
with a checklist, although it's mostly not very interesting.
Test plan: read recent issues
This means we can use generics and various other things. I didn't use
any of them yet, this is just bumping the numbers. I added tests for
1.20, and fixed one small bug, I think caused by `go/packages` changes
therein. And I bumped the golangci-lint version too while I was in the
area.
Fixes#256, fixes#257.
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
If you do `UPDATE_SNAPSHOTS=1 go test ./...` that will:
1. run the snapshot tests, updating any changed snapshots
2. run the integration tests and (if you have tokens) example tests
3. check that the code for the integration tests and example tests is
up-to-date
Step 3 is not strictly a snapshot test; the generated code is actually
checked in and used. But, I mean, it's basically the same! So now, we
also update it if you asked to update snapshots, which is hopefully a
little more convenient.
Fixes#212.
Test plan:
Make a trivial change to `example/generated.go`; `go test ./...` should
now fail. `UPDATE_SNAPSHOTS=1 go test ./...` should also fail, but say
it updated the snapshot, and the change should be reverted. Run `go test
./...` again; it should pass again.
Support package-names with dashes in them
We were smart about aliasing if you have name-collisions, but not if
your package name is something that's not a valid identifier, like
`"path/to/my-package"`, which Go for better or worse allows. Now we
remove all the invalid characters (in practice mainly dashes, dots, and
leading digits).
Fixes#231.
Test plan: make check
A couple people noticed the documentation didn't match
the actual option syntax we settled on. Now it does.
Fixes#226, replaces #222 (closed due to CLA issues).
Thanks to `git merge` being clever, this got put in `v0.5.0` even
though it was added after `v0.5.0` was released. Now it's in `vNext`
(perhaps soon to become `v0.5.1`).
The entries under the new `package_bindings` field should be packages,
but it's an easy mistake to put a file path instead (most of the other
fields in `genqlient.yaml` are files). Due to some bizzare behavior from
`go/packages` (described in #220), if you do that you get weird broken
code that gives you no clue what is wrong. Instead, let's guess if what
you gave us looks like a filename, and report a nice error if so.
Test plan: crossed fingers
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
The generated files for integration tests aren't strictly snapshots and so
`UPDATE_SNAPSHOTS=1` won't work (maybe we should make it work?).
Instead you also need to `go generate ./...`. This came up in #209.
Frequent contributors and those adding significant new functionality
will want to read all the comments in `generate_test.go`, but people
making a small fix just want to update the snapshots. So it makes
sense to put the formula for doing so directly in the contributor docs.
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
GraphQL schemas have some builtin types, like `String`. The spec says
your SDL must not include those, but in practice some schemas do. (This
is probably because introspection must include them, and some tools that
create SDL from introspection don't know they're supposed to filter them
out.) Anyway, we've since #145 had logic to handle this; we just parse
with and without the prelude that defines them and see which works.
The problem is that this makes for very confusing error messages if you
have an invalid schema. (Or if you have a schema that you think is valid
but gqlparser doesn't, which is the more common case in the wild; see
for example #200.) Right now if both ways error we take the
without-prelude error, which if you didn't define the builtins is just
`undefined type String`; if we took the with-prelude error then if you
did define the builtins you'd just get `type String defined twice`. So
we actually have to be smart if we want good error messages for
everyone.
So in this commit we are smart: we check if your schema defines
`String`, and include the prelude only if it does not. To do this I
basically inlined `gqlparser.LoadSchema` (twice), so that in between
parsing and validation we can check if you have `String` and if not add
the prelude. This should in theory be both more efficient (we don't
have to do everything twice) and give better error messages,
although it's a bit more code.
Fixes#175.
Test plan: make check
I didn't realize until today that implementations of GraphQL interfaces
are actually allowed to be covariant: if the interface has a field
`f: T`, then the implementations may have fields `f: U` where `U` is a
subtype of `T` (for example `U` may be an implementation of the
interface `T`, or `U` may be `T!` if `T` is non-nullable. (I thought it
had to be `f: T` exactly.) So I figured I'd add a test and see what
breaks.
Surprisingly, and despite the fact that Go interfaces do *not* allow
covariance, everything... worked? There's at least one place where it's
possible we could ideally use a more specific type [1], but for now I
just wanted to make sure we at least write something that builds and is
vaguely reasonable. Of course I'm not sure if there's anything I've
missed (some day I need to find a fuzzing engine that can fuzz GraphQL).
[1] Specifically, the field
`CovariantInterfaceImplementationRandomItemTopic.Next` might ideally
have type `...NextContentTopic`, not `...NextContent`; we know it's a
topic. This doesn't directly cause covariance problems in Go: the method
`GetNext` still returns `...NextContent` so the interface matches. But
that trick doesn't work for the sibling field `.Related` which is
slice-typed: or rather, we'd need the method to copy the slice to the
correct type. (Not to mention the implemention of any change here would
require a bunch of plumbing because the AST doesn't quite have what we
want.) So it's probably best to just keep this as-is for simplicity and
consistency.
Test plan: make check
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
Some of the errors tests need to have their own schema, so the schema
can do something weird (or even be entirely invalid!). But most can
still share a schema. In this commit I have those indeed share a
schema, to avoid having to have a bunch of copies of mostly the same
schema. While doing so I noticed one error whose location wasn't very
useful, and fixed it.
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
