Khan / genqlient

a truly type-safe Go GraphQL client
MIT License
1.07k stars 107 forks source link

Add "generic" option to the "optional" configuration for handling nullable types #252

Closed DylanRJohnston closed 1 year ago

DylanRJohnston commented 1 year ago

This is an implementation for #251, it adds a new "generic" option for the "optional" configuration, and a companion type "optional_generic_type" which is a fully qualified type with a placeholder % for the generic parameter.

I have tested this locally with a custom optional.Value[A] implementation but I'm unsure how to add tests for this without bumping the Go version from 1.16 to 1.18. I would appreciate any input on how best to achieve this. Interested in any feedback on the design before I fill out the details on documentation and changelog.

I have:

DylanRJohnston commented 1 year ago

Here's an example of it in use https://github.com/DylanRJohnston/genqlient/compare/main...DylanRJohnston:genqlient:example

benjaminjkraft commented 1 year ago

I love the concept, thanks very much for implementing it! I don't have time for a proper review today but one thing I see is we'll want tests for this -- there are snapshot tests already set up and you'll just have to make some way to skip these on versions below 1.18. I'm a bit busy right now but I'll try to get back with a full review within a week or so.

benjaminjkraft commented 1 year ago

Actually, one more thought on integration tests: I'm ok just running all the integration tests only against 1.18+, I don't think there's much we really need to test against older Go versions there. Not sure if that will be enough to make it easy though, so what I said before still applies.

DylanRJohnston commented 1 year ago

@benjaminjkraft I've added a snapshot test and some more documentation. Do you want the the entire integration test to be updated to use the testutil.Option or to just leave it for now? Also should the change log be updated now, or only when do you a release?

benjaminjkraft commented 1 year ago

Ok, I pulled the version upgrade into #257, so ~once that merges~ you should be able to just merge and avoid dealing with that!

benjaminjkraft commented 1 year ago

I went ahead and merged main and added the changelog. Thanks again for writing this -- I'm planning on using it in a project I'm working on!

DylanRJohnston-FZ commented 1 year ago

Thanks! Apologies for not pushing this over the line myself. I just got absolutely swamped at work just after putting this pull request up. Glad to see it in main though!