Reject the use of both typename and bind #172
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
benjaminjkraft
requested review from
csilvers,
dnerdy and
StevenACoffman
as code owners
Closed
csilvers
approved these changes
Feb 10, 2022
|
Yeah, I'm only 90% sure I thought of all the ways it could happen, but I decided that's way better than 0%! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 willgenerate and use
type MyString string. But this gets a bit confusingif you mix it with
bind; shouldtypename: "MyString", bind: "int32"generatetype MyString int32, orshould 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
fordirective, and in that case probably it was amistake. 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
typenamewin over a global binding bylocally unbinding it via
bind: "-".) This required changes insurprisingly 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
I have: