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

[DylanRJohnston]

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:

• Written a clear PR title and description (above)
• Signed the Khan Academy CLA
• Added tests covering my changes, if applicable
• Included a link to the issue fixed, if applicable
• Included documentation, for new features
• Added an entry to the changelog

[DylanRJohnston]

Here's an example of it in use DylanRJohnston/genqlient@main...DylanRJohnston:genqlient:example

[benjaminjkraft]

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]

Cool, this all seems reasonable to me once it has tests and a bit of documentation! I don't love the config syntax having two separate options but I don't have any better ideas right now so let's go with it. The only other thing I wonder is whether trying to substitute in a % in an arbitrary expression is going to be more confusing than helpful -- for example if you do path/to.MyWeirdOptional[path/to.MyArg, %] it probably(?) won't work because we don't know how to do the import. So I wonder if we want to just say it has to be a generic with one argument, and you just say the name of the type (path/to.Optional).

For the tests, thinking a bit more and I think what you can do is something like:

• For the generate tests, just check runtime.Version and skip the tests if it's too low (feel free to do this in a mildly hacky way, like a list of test case names).
• For the integration tests, I guess it's tricky because they all share one generated file. Maybe make a separate subpackage with a copy of the wiring, genqlient.config, etc., and put //go:build lines in each file? Although even that might be awkward because we don't have a great way to put //go:build in the generated.go itself, and I'm guessing it will break things like go generate ./... if we can't parse arbitrary files. And we also don't have a great setup for integration testing different configs. Honestly maybe it's not worth integration testing this right now, you could just leave that out for now, or run it manually and then comment it out so we can hopefully add it back when we're ready to only support 1.18+ (hopefully soon since 1.20 is out).

[benjaminjkraft]

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]

@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]

Sorry for the delay in reviewing. This seems great!

It's not ideal to have this without integration tests, but I think it's gonna be a decent amount of work to add the support. I've created #261 for this; for now I think it's fine if you just want to test it manually in some other project and that plus snapshot tests should be good enough. Do make sure to at least test it manually as marshaling can go wrong in surprising places!

I suspect that you'll have to either fully drop support for 1.18 (which means removing it from the list of versions we run tests on as well) or put the additions to types.go behind a build flag and add a flag to the test itself. Probably we can just drop 1.18 and that will be easier. (Or if I have time I can do the version changes in a separate PR first, but it's not super likely I will this week or next.)

Beyond that (and one question for your thoughts below), I think all we need is the changelog entry (or two, if dropping 1.18)!

[benjaminjkraft]

maybe also complain if c.Optional != "generic" && c.OptionalGenericType != ""?

[benjaminjkraft]

It seems like this is duplicated now? Did you edit this manually or are the go tools doing something weird? (I'm guessing this is also the source of the merge conflict.)

[DylanRJohnston-FZ]

This is auto generated by go tidy. My editor was complaining it needed to be run, but I'm happy to remove ti.

[benjaminjkraft]

hmm, I wonder if this (which I believe corresponds to a GraphQL [String], i.e. optional list of optional strings) should be testutil.Option[[]testutil.Option[string]]. We don't do that for pointers because it's annoying in practice, but if you're using an Option type it might be convenient and make it clearer for users to distinguish between [] and null using Option. What do you think?

[DylanRJohnston-FZ]

Generally speaking I prefer to use an empty array instead of option where I can because empty arrays have nice ergonomics, however you may want this in a situation where you're trying to set something to an empty array and it gets interpreted as a missing value. However I've never come across such a situation personally.

[benjaminjkraft]

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]

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]

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!