Refactor import-loading to simplify the type-generation code #101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Reviewers: marksandstrom, mahtab, jvoll, adam, miguel, steve
benjaminjkraft
requested review from
aberkan,
dnerdy,
jvoll,
mahtabsabet,
MiguelCastillo and
StevenACoffman
StevenACoffman
approved these changes
Sep 16, 2021
dnerdy
approved these changes
Sep 16, 2021
generate/generate.go
Outdated
| for _, op := range document.Operations { | ||
| if err = g.addOperation(op); err != nil { | ||
| for _, operation := range g.Operations { | ||
| err = g.execute("operation.go.tmpl", &bodyBuf, operation) |
There was a problem hiding this comment.
I know "execute" is based on text/template terminology, but that isn't super clear in the context of the generator, IMO. Perhaps this function could be named renderTemplate or something similar.
generate/imports.go
Outdated
| if !ok { | ||
| if g.importsLocked { | ||
| return "", errorf(nil, | ||
| `genqlient internal error: imports locked but no alias defined for package "%v"`, pkgPath) |
There was a problem hiding this comment.
perhaps: "imports locked but package "%v" has not been imported"?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
genqlient has some code (
imports.go) dedicated to tracking whichimports we need and avoiding conflicts, as well as converting a
(restricted) Go expression like
map[string]github.com/me/mypkg.MyTypeto 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:needed, and returns the reference either way
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 (Allow mapping directly to a type but not using its standard JSON serialization #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