Add support for specifying type-names, and conflict-detection #94
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
benjaminjkraft
requested review from
aberkan,
csilvers,
dnerdy,
jvoll,
mahtabsabet and
MiguelCastillo
This was
linked to
issues
Sep 15, 2021
StevenACoffman
approved these changes
Sep 15, 2021
jvoll
approved these changes
Sep 15, 2021
docs/FAQ.md
Outdated
| type: time.Time | ||
| ``` | ||
|
|
||
| Make sure the given type has whateevr logic is needed to convert to/from JSON (e.g. `MarshalJSON`/`UnmarshalJSON` or JSON tags). See the [`genqlient.yaml` documentation](genqlient.yaml) for the full syntax. |
There was a problem hiding this comment.
Suggested change
| Make sure the given type has whateevr logic is needed to convert to/from JSON (e.g. `MarshalJSON`/`UnmarshalJSON` or JSON tags). See the [`genqlient.yaml` documentation](genqlient.yaml) for the full syntax. | |
| Make sure the given type has whatever logic is needed to convert to/from JSON (e.g. `MarshalJSON`/`UnmarshalJSON` or JSON tags). See the [`genqlient.yaml` documentation](genqlient.yaml) for the full syntax. |
generate/convert.go
Outdated
|
|
||
| expectedSelectionSet := typ.SelectionSet() | ||
| if err := selectionsMatch(pos, selectionSet, expectedSelectionSet); err != nil { | ||
| fmt.Println(goName, err) |
There was a problem hiding this comment.
Did you intend to both print + return the error?
There was a problem hiding this comment.
I sure didn't!
In this commit I add two related features to genqlient: conflict-detection to avoid generating two distinct types with the same name, and an option to specify the type-name genqlient should use for some type. The conflict-detection was pretty simple once I realized I had already written all the code to do it in #70. There was a bunch of wiring, since we now need to keep track of the GraphQL type/selection-set that each type corresponds to, but it was pretty straightforward. This allows us to: - detect and reject if you have really sneaky type-names (there are some examples documented in `names.go`) - more clearly crash if genqlient accidentally generates two conflicting types, and - avoid stack-overflow when handing recursive (input) types (although sadly the poor support for options on input types (#14) makes them difficult to use in many cases; you really need to be able to set `pointer: true`) And with that all set up, the type-naming was also easy! (It doesn't have to get into the core of the type-generator, just plug in where we choose names. The desire for conflict detection was the main reason I hadn't set it up already.) Note that the existing limitation of #70 that the fields have to be in exactly the same order remains (and is now documented as #93); it's not deeply hard to fix but it's surprisingly much work. Issue: #60 Issue: #12 Test plan: make check Reviewers: csilvers, marksandstrom, adam, miguel, jvoll, mahtab
benjaminjkraft
force-pushed
the
benkraft.type-name
branch
from
b494d94
to
9a3bef4
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:
In this commit I add two related features to genqlient:
conflict-detection to avoid generating two distinct types with the same
name, and an option to specify the type-name genqlient should use for
some type.
The conflict-detection was pretty simple once I realized I had already
written all the code to do it in #70. There was a bunch of wiring,
since we now need to keep track of the GraphQL type/selection-set that
each type corresponds to, but it was pretty straightforward. This
allows us to:
examples documented in
names.go)types, and
sadly the poor support for options on input types (Improve options-handling for input types #14) makes them
difficult to use in many cases; you really need to be able to set
pointer: true)And with that all set up, the type-naming was also easy! (It doesn't
have to get into the core of the type-generator, just plug in where we
choose names. The desire for conflict detection was the main reason I
hadn't set it up already.) Note that the existing limitation of #70 that
the fields have to be in exactly the same order remains (and is now
documented as #93); it's not deeply hard to fix but it's surprisingly
much work.
Issue: #60
Issue: #12
Test plan:
make check