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

resolvergen: use the resolver type as base name for dependent types #728

Conversation

fgallina
Copy link
Contributor

@fgallina fgallina commented May 26, 2019

The template was outputing invalid code since the resolver type was
not used in places like the embedding at {query,mutation}Resolver.

This change also ensures that objects like {query,mutation}Resolver
also use the user provided type name as suffix.

Here's the resulting diff on the code generation with type: GeneratedResolver in the resolver config:

diff -u resolver.go resolvernew.go
--- resolver.go 2019-05-26 20:04:15.361969755 -0300
+++ resolvernew.go      2019-05-26 20:04:54.170737786 -0300
@@ -7,20 +7,20 @@
 type GeneratedResolver struct{}

 func (r *GeneratedResolver) Mutation() MutationResolver {
-       return &mutationResolver{r}
+       return &mutationGeneratedResolver{r}
 }
 func (r *GeneratedResolver) Query() QueryResolver {
-       return &queryResolver{r}
+       return &queryGeneratedResolver{r}
 }

-type mutationResolver struct{ *Resolver }
+type mutationGeneratedResolver struct{ *GeneratedResolver }

-func (r *mutationResolver) CreateTodo(ctx context.Context, input NewTodo) (*Todo, error) {
+func (r *mutationGeneratedResolver) CreateTodo(ctx context.Context, input NewTodo) (*Todo, error) {
        panic("not implemented")
 }

-type queryResolver struct{ *Resolver }
+type queryGeneratedResolver struct{ *GeneratedResolver }

-func (r *queryResolver) Todos(ctx context.Context) ([]*Todo, error) {
+func (r *queryGeneratedResolver) Todos(ctx context.Context) ([]*Todo, error) {
        panic("not implemented")
 }

I haven't yet found the place where this is tested nor the sensible way to add test for this use case. Will update as soon as I find it.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@mathewbyrne
Copy link
Contributor

Can you please add a test case demonstrating the issue? It looks like none of the generated code has changed from to your update.

@fgallina fgallina force-pushed the make-generated-resolver-dependent-types-follow-configured-type branch from 8fa2e5b to 24b40dc Compare June 12, 2019 23:34
@fgallina
Copy link
Contributor Author

@mathewbyrne I think that may be related to the fact that the resolver.go generation is skipped if the file already exists. Also there must be a custom resolver type in the gqlgen.yml to exercise these changes. Excerpt from my original config that drove me to propose this change:

...
resolver:
  filename: graphql/resolver.go
  package: graphql
  type: GeneratedResolver

The template was outputing invalid code since the resolver type was
not used in places like the embedding at {query,mutation}Resolver.

This change also ensures that objects like {query,mutation}Resolver
also use the user provided type name as suffix.

Here's the resulting diff on the code generation with `type:
GeneratedResolver` in the resolver config:

```
diff -u resolver.go resolvernew.go
--- resolver.go 2019-05-26 20:04:15.361969755 -0300
+++ resolvernew.go      2019-05-26 20:04:54.170737786 -0300
@@ -7,20 +7,20 @@
 type GeneratedResolver struct{}

 func (r *GeneratedResolver) Mutation() MutationResolver {
-       return &mutationResolver{r}
+       return &mutationGeneratedResolver{r}
 }
 func (r *GeneratedResolver) Query() QueryResolver {
-       return &queryResolver{r}
+       return &queryGeneratedResolver{r}
 }

-type mutationResolver struct{ *Resolver }
+type mutationGeneratedResolver struct{ *GeneratedResolver }

-func (r *mutationResolver) CreateTodo(ctx context.Context, input NewTodo) (*Todo, error) {
+func (r *mutationGeneratedResolver) CreateTodo(ctx context.Context, input NewTodo) (*Todo, error) {
        panic("not implemented")
 }

-type queryResolver struct{ *Resolver }
+type queryGeneratedResolver struct{ *GeneratedResolver }

-func (r *queryResolver) Todos(ctx context.Context) ([]*Todo, error) {
+func (r *queryGeneratedResolver) Todos(ctx context.Context) ([]*Todo, error) {
        panic("not implemented")
 }
```
@fgallina fgallina force-pushed the make-generated-resolver-dependent-types-follow-configured-type branch from 24b40dc to c5acbea Compare June 15, 2019 01:31
}

t.Run("renders expected contents", func(t *testing.T) {
m := &Plugin{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an easier and simpler way to write this test would be to use golden files (https://medium.com/@jarifibrahim/golden-files-why-you-should-use-them-47087ec994bf)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already do this using code review of the committed generated files. No need for update, just commit the generated code and push.

All this test should need to do is call the generator with some config, eg. It may not look like its asserting anything, but really its output is tested in three ways:

  • firstly we run goimports when saving go files, so running generate should error out in the test if it generates invalid code.
  • if that fails the linter should see invalid code, and fail the branch.
  • code review catches changes. diffs are readable in github.

@@ -19,6 +19,7 @@ type Plugin struct{}
var _ plugin.CodeGenerator = &Plugin{}

func (m *Plugin) Name() string {
// TODO: typo, should be resolvergen
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

}

t.Run("renders expected contents", func(t *testing.T) {
m := &Plugin{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already do this using code review of the committed generated files. No need for update, just commit the generated code and push.

All this test should need to do is call the generator with some config, eg. It may not look like its asserting anything, but really its output is tested in three ways:

  • firstly we run goimports when saving go files, so running generate should error out in the test if it generates invalid code.
  • if that fails the linter should see invalid code, and fail the branch.
  • code review catches changes. diffs are readable in github.

@vektah vektah merged commit 7fed71b into 99designs:master Jun 24, 2019
@vektah vektah added the v0.9.1 fixed in v0.9.1 label Jun 27, 2019
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
…r-dependent-types-follow-configured-type

resolvergen: use the resolver type as base name for dependent types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v0.9.1 fixed in v0.9.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants