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

New option to make comments on resolver optional #2627

Merged
merged 4 commits into from
May 2, 2023

Conversation

mokeko
Copy link
Contributor

@mokeko mokeko commented Apr 29, 2023

After the PR #2263 , the gqlgen generate command started generating comments such as // Todos is the resolver for the todos field. above resolver implementations. However, these comments are not necessary for me. So I added an option to not generate such comments.

When omit_template_comment: true is set, the above comment will not be generated. However, pre-written comments are retained after the code re-generation. This feature is necessary to keep manually written comments.

Relevent issues and PRs: #2413 , #2617

I have:

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

@@ -68,16 +68,17 @@ func (m *Plugin) generateSingleFile(data *codegen.Data) error {
continue
}

resolver := Resolver{o, f, nil, "// foo", `panic("not implemented")`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, a fixed comment // // foo is generated for all resolvers in single-file mode, but it appears to me as if it was accidentally added to the code during development and was not intentional. Therefore, I have made a modification to generate the same comments as in follow-schema mode.

@mokeko mokeko marked this pull request as ready for review April 29, 2023 06:35
@coveralls
Copy link

Coverage Status

Coverage: 78.751% (+0.05%) from 78.705% when pulling a699303 on mokeko:omit-resolver-auto-comment into 239b97e on 99designs:master.

@StevenACoffman
Copy link
Collaborator

I'm not at all sure why, but I'm repeatedly getting some test failures under windows and Go 1.20 which seem related to websockets. Given that the rest of the matrix passed, I'm imagining it's just a flaky test, but that's what's holding me up from merging this.

ok  	github.com/99designs/gqlgen/graphql/handler/extension	1.071s
2023/05/02 15:41:29 decoding error: invalid character 'o' in literal null (expecting 'u') body:notjson
--- FAIL: TestWebsocketWithPingPongInterval (0.03s)
    --- FAIL: TestWebsocketWithPingPongInterval/client_receives_ping_and_responds_with_pong (0.03s)
panic: read tcp 127.0.0.1:54980->127.0.0.1:54979: wsarecv: An established connection was aborted by the software in your host machine. [recovered]
	panic: read tcp 127.0.0.1:54980->127.0.0.1:54979: wsarecv: An established connection was aborted by the software in your host machine.

goroutine 266 [running]:
testing.tRunner.func1.2({0x9e9460, 0xc000228dc0})
	C:/hostedtoolcache/windows/go/1.20.3/x64/src/testing/testing.go:1526 +0x372
testing.tRunner.func1()
	C:/hostedtoolcache/windows/go/1.20.3/x64/src/testing/testing.go:1529 +0x650
panic({0x9e9460, 0xc000228dc0})
	C:/hostedtoolcache/windows/go/1.20.3/x64/src/runtime/panic.go:890 +0x263
github.com/99designs/gqlgen/graphql/handler/transport_test.readOp(...)
	D:/a/gqlgen/gqlgen/graphql/handler/transport/websocket_test.go:666
github.com/99designs/gqlgen/graphql/handler/transport_test.TestWebsocketWithPingPongInterval.func2(0x0?)
	D:/a/gqlgen/gqlgen/graphql/handler/transport/websocket_test.go:593 +0x6dc
testing.tRunner(0xc000234340, 0xc000170ae0)
	C:/hostedtoolcache/windows/go/1.20.3/x64/src/testing/testing.go:1576 +0x217
created by testing.(*T).Run
	C:/hostedtoolcache/windows/go/1.20.3/x64/src/testing/testing.go:1629 +0x806
FAIL	github.com/99designs/gqlgen/graphql/handler/transport	0.755s
ok  	github.com/99designs/gqlgen/graphql/introspection	1.053s
?   	github.com/99designs/gqlgen/handler	[no test files]
ok  	github.com/99designs/gqlgen/graphql/playground	1.062s
?   	github.com/99designs/gqlgen/integration	[no test files]
?   	github.com/99designs/gqlgen/integration/models-go	[no test files]
?   	github.com/99designs/gqlgen/integration/remote_api	[no test files]
?   	github.com/99designs/gqlgen/integration/server	[no test files]
?   	github.com/99designs/gqlgen/integration/testomitempty	[no test files]
?   	github.com/99designs/gqlgen/plugin	[no test files]
ok  	github.com/99designs/gqlgen/internal/code	5.010s
ok  	github.com/99designs/gqlgen/internal/imports	1.504s
ok  	github.com/99designs/gqlgen/internal/rewrite	1.688s
?   	github.com/99designs/gqlgen/plugin/federation/fedruntime	[no test files]
?   	github.com/99designs/gqlgen/plugin/federation/test_data/model	[no test files]
?   	github.com/99designs/gqlgen/plugin/modelgen/out	[no test files]
?   	github.com/99designs/gqlgen/plugin/modelgen/out_nullable_input_omittable	[no test files]
?   	github.com/99designs/gqlgen/plugin/modelgen/out_struct_pointers	[no test files]
?   	github.com/99designs/gqlgen/plugin/servergen	[no test files]
?   	github.com/99designs/gqlgen/plugin/stubgen	[no test files]
ok  	github.com/99designs/gqlgen/plugin/federation	110.303s
ok  	github.com/99designs/gqlgen/plugin/federation/fieldset	1.302s
==3872==ERROR: ThreadSanitizer failed to allocate 0x000001000000 (16777216) bytes at 0x0400b2000000 (error code: 1455)
FAIL	github.com/99designs/gqlgen/plugin/modelgen	[33](https://github.com/99designs/gqlgen/actions/runs/4837354097/jobs/8670142701?pr=2627#step:4:34).9[41](https://github.com/99designs/gqlgen/actions/runs/4837354097/jobs/8670142701?pr=2627#step:4:42)s
ok  	github.com/99designs/gqlgen/plugin/resolvergen	[53](https://github.com/99designs/gqlgen/actions/runs/4837354097/jobs/8670142701?pr=2627#step:4:54).0[64](https://github.com/99designs/gqlgen/actions/runs/4837354097/jobs/8670142701?pr=2627#step:4:65)s

@StevenACoffman
Copy link
Collaborator

Ok, weird. 10th time was the charm, and now it finally passes for some reason. Anyone who wants to make that more reliable, I would be extremely grateful!

@StevenACoffman StevenACoffman merged commit 395c362 into 99designs:master May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants