Allow the use of typename to be combined with bind.
#138
Conversation
In that case, we make a type-alias that points to the `bind` type, and use that type-alias for the type of the field.
csilvers
requested review from
benjaminjkraft,
dnerdy and
StevenACoffman
as code owners
|
@benjaminjkraft here's the diff, as suggested in #133. There's a bit of code duplication, but I couldn't tell if it was worth trying to extract it out into a shared function, it seemed like it wouldn't actually help that much. I'm wondering if it would make sense to try to combine the local vs global I couldn't figure out if there were existing tests of the global bind option that I could hook into, to test the change for that. If there are I didn't see them! |
There was a problem hiding this comment.
Oh, yeah, this is a bit more awkward than anticipated. I was thinking you would do it all on line 261 (in convertType by GetPointer()), replacing both the existing copy and the two added, but I see that doesn't work for local bindings (although maybe if you add an else to that if it could?) and would require type-asserting to see if what you got was an opaque type (maybe fine).
I also realize that even moreso than I thought, this will be a bit confusing because named types don't inherit the JSON-marshaling. Maybe it's better to just reject the combination, since it's probably not what you intended. If people find use cases then we can reintroduce it.
I'm ok either way. I think if you keep this as-is and aren't able to put it all in one place, it would be good to wrap it into a helper method, but it's not a big deal either way.
There was a problem hiding this comment.
Oh and for tests there are a bunch of bindings set up, for ID, Pokemon, etc.: https://github.com/Khan/genqlient/blob/main/generate/generate_test.go#L85
| # and we'll fetch it a second time, again making it a list of strings | ||
| # but creating our own type-name for that list of strings. | ||
| # @genqlient(bind: "[]string", typename: "StringList") | ||
| rolesAgain: roles | ||
| name | ||
| # this is mapped globally to internal/testutil.Pokemon: | ||
| # note field ordering matters, but whitespace shouldn't. |
Ooh yeah, that's a good point about the marshaling. I think you're right it's better to just reject it for now, it doesn't seem very useful. I'll do that when I have a chance. |
Closing this as a result. We should probably detect and reject this situation, but that can be a separate PR. |
In #133, Craig added support for a new use of typename, where it applies to a scalar and means that genqlient should generate a named type, e.g. `# @genqlient(typename: "MyString")` on a node of type string will generate and use `type MyString string`. But this gets a bit confusing if you mix it with `bind`; should `typename: "MyString", bind: "int32"` generate `type MyString int32`, or should one override the other, or what? Of course in practice you're not likely to write that all in one place, but you could via a global binding, or a `for` directive, and in that case probably it was a mistake. In #138, we looked at making them work together correctly, but it added complexity and got even more confusing. So instead, here, we just ban it; we can always add it back if it proves useful. (Or, you can make the `typename` win over a global binding by locally unbinding it via `bind: "-"`.) This required changes in surprisingly many places; I already knew the directive-validation code was due for a refactor but that will happen some other day. The tests show that it works, in any case. Interestingly, this problem actually could have arisen for a struct binding already, before #133. But all the same reasons it's confusing seem to apply, so I just banned it there too. This is technically a breaking change although I doubt anyone will hit it. Test plan: make check
In #133, Craig added support for a new use of typename, where it applies to a scalar and means that genqlient should generate a named type, e.g. `# @genqlient(typename: "MyString")` on a node of type string will generate and use `type MyString string`. But this gets a bit confusing if you mix it with `bind`; should `typename: "MyString", bind: "int32"` generate `type MyString int32`, or should one override the other, or what? Of course in practice you're not likely to write that all in one place, but you could via a global binding, or a `for` directive, and in that case probably it was a mistake. In #138, we looked at making them work together correctly, but it added complexity and got even more confusing. So instead, here, we just ban it; we can always add it back if it proves useful. (Or, you can make the `typename` win over a global binding by locally unbinding it via `bind: "-"`.) This required changes in surprisingly many places; I already knew the directive-validation code was due for a refactor but that will happen some other day. The tests show that it works, in any case. Interestingly, this problem actually could have arisen for a struct binding already, before #133. But all the same reasons it's confusing seem to apply, so I just banned it there too. This is technically a breaking change although I doubt anyone will hit it. Test plan: make check
Summary:
In that case, we make a type-alias that points to the
bindtype, anduse that type-alias for the type of the field.
Test plan:
make check