Fix type-naming in the presence of interfaces, and refactor it a lot #71
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
|
This is a fixed version of #66. |
benjaminjkraft
force-pushed
the
benkraft.fragments-1
branch
from
857294f to
28d7420
Compare
benjaminjkraft
force-pushed
the
benkraft.fragments-2
branch
from
47014d5 to
d2c6b1b
Compare
|
(Plus, now, a rebase!) |
dnerdy
approved these changes
Aug 28, 2021
Comment on lines
+3
to
+6
| // This file generates the names for genqlient's generated types. This is | ||
| // somewhat tricky because the names need to be unique, stable, and, to the | ||
| // extent possible, human-readable and -writable. See DESIGN.md for an | ||
| // overview of the considerations; in short, we need long names. |
There was a problem hiding this comment.
I said it in #66, but I'll say it again. Amazing documentation! Thanks.
generate/names.go
Outdated
| // easy. | ||
| type prefixList struct { | ||
| last string // the list goes back-to-front, so this is the *last* prefix | ||
| rest *prefixList |
There was a problem hiding this comment.
nit: perhaps previous instead of rest? Or maybe name last either prefix or name or element and say this is a forward linked list but with the elements in the opposite order?
There was a problem hiding this comment.
Ah, good ideas.
| } | ||
| l := len(reversed) | ||
| for i := 0; i < l/2; i++ { | ||
| reversed[i], reversed[l-1-i] = reversed[l-1-i], reversed[i] |
There was a problem hiding this comment.
I wish I did not have to know that this is the standard way to reverse a list in Go!
generate/convert.go
Outdated
| // thing, because we want to do a few things differently, and because we | ||
| // know we have an object type, so we can include only that case. | ||
| fields, err := g.convertSelectionSet( | ||
| &prefixList{last: operation.Name}, operation.SelectionSet, baseType, queryOptions) |
There was a problem hiding this comment.
It seems like a prefixList constructor could be useful so the caller doesn't need to know anything about the internal structure of the list.
When adding support for interfaces, I did not do the type-names as I intended: they came out to be `MyFieldMyType`, not `MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly wrong. But once supporting fragments, this is also now incorrect. (Exactly why is described in the comments inline.) In this commit, in any case, I fix it. To do that, I finally did the last of the refactors I've been hoping to do but unable to successfully implement, which is to make the type-name and type-name-prefix management clearer. In the past it was kind of spread out, and each caller would have to pass the right name into `convertDefinition`, which go quite unwieldy. Now, the case that really wanted that -- the operation toplevel -- just does it own thing; and the main name-generation code is factored out into a separate file with tests, and with a long comment that goes into all the details of the algorithm that the design-doc didn't cover. (I even had some fun using a linked list to implement the prefix-stack!) This allowed me to fix the above bug fairly easily -- actually the fix was pretty much automatic once I understood how to organize things. There is one change which is that if your query name is unexported, we no longer do the same with the input-type names; it's unclear to me if anyone will actually care about this behavior (Khan always makes the queries exported) but if they did it was very inconsistent (only at the query toplevel, and only for input-objects, not enums), so we can reimplement it properly if that comes up. As a bonus fix, we now better handle the case where your type-names are lowercase, which is legal if nonstandard GraphQL. Issue: #8 Test plan: make tesc Reviewers: marksandstrom, adam, miguel
benjaminjkraft
force-pushed
the
benkraft.fragments-2
branch
from
d2c6b1b to
72f8e07
Compare
benjaminjkraft
added a commit
that referenced
this pull request
Sep 22, 2021
We typically name our types `OperationFieldTypeFieldType`, but if a type's name matches the preceding field-name, we omit the type-name. In #71 I changed the behavior such that we no longer do that in the case where the type's name matches some suffix of the name-so-far that's longer than just the leaf field-name. This was semi-intentional; I assumed it didn't matter and would be more predictable this way. But it turns out that was a feature, both in the sense that almost any change to the type-name-generator is breaking, and in the sense that it made the names uglier. Plus, now that we have better conflict-detection (#94), the possibility that some tricksy type-names could cause problems is no longer as much of an issue, so we can be a little less careful here. (Although I think this is no less safe than before; the field-names are the important part.) So in this commit I revert the change. Specifically, this comes up a lot at Khan where we do ``` mutation ForcePhantom { forcePhantom { # type: ForcePhantom error { ... } # type: ForcePhantomError } } ``` Before #71, and again after this change, we'll generate `ForcePhantomForcePhantomError` for `error`; before we'd generate `ForcePhantomForcePhantomErrorForcePhantomError`. Issue: #109 Test plan: make tesc Reviewers: csilvers, marksandstrom, steve, mahtab, adam, miguel, jvoll
benjaminjkraft
added a commit
that referenced
this pull request
Sep 23, 2021
## Summary: We typically name our types `OperationFieldTypeFieldType`, but if a type's name matches the preceding field-name, we omit the type-name. In #71 I changed the behavior such that we no longer do that in the case where the type's name matches some suffix of the name-so-far that's longer than just the leaf field-name. This was semi-intentional; I assumed it didn't matter and would be more predictable this way. But it turns out that was a feature, both in the sense that almost any change to the type-name-generator is breaking, and in the sense that it made the names uglier. Plus, now that we have better conflict-detection (#94), the possibility that some tricksy type-names could cause problems is no longer as much of an issue, so we can be a little less careful here. (Although I think this is no less safe than before; the field-names are the important part.) So in this commit I revert the change. Specifically, this comes up a lot at Khan where we do ``` mutation ForcePhantom { forcePhantom { # type: ForcePhantom error { ... } # type: ForcePhantomError } } ``` Before #71, and again after this change, we'll generate `ForcePhantomForcePhantomError` for `error`; before we'd generate `ForcePhantomForcePhantomErrorForcePhantomError`. Issue: #109 ## Test plan: make tesc Author: benjaminjkraft Reviewers: csilvers, aberkan, dnerdy, jvoll, mahtabsabet, MiguelCastillo, StevenACoffman Required Reviewers: Approved By: csilvers Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint Pull Request URL: #110
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.
Summary:
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be
MyFieldMyType, notMyInterfaceMyFieldMyType, which is inconsistent, but not strictlywrong. But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.) In this commit, in
any case, I fix it.
To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer. In the past it was kind of
spread out, and each caller would have to pass the right name into
convertDefinition, which go quite unwieldy. Now, the case that reallywanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover. (I even had some fun using
a linked list to implement the prefix-stack!)
This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up. As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.
Issue: #8
Test plan:
make tesc