Add support for interfaces, part 4: getter methods #57
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
force-pushed
the
benkraft.interfaces-3
branch
from
807a91a to
1771758
Compare
benjaminjkraft
force-pushed
the
benkraft.interfaces-4
branch
2 times, most recently
from
a62553b to
0965329
Compare
benjaminjkraft
force-pushed
the
benkraft.interfaces-3
branch
from
1771758 to
1821951
Compare
benjaminjkraft
force-pushed
the
benkraft.interfaces-4
branch
from
8352001 to
903f3af
Compare
benjaminjkraft
force-pushed
the
benkraft.interfaces-3
branch
from
1821951 to
ef831b4
Compare
benjaminjkraft
force-pushed
the
benkraft.interfaces-4
branch
from
903f3af to
4e8b2d7
Compare
benjaminjkraft
commented
Aug 25, 2021
| @@ -116,11 +137,25 @@ func TestInterfaceListField(t *testing.T) { | |||
|
|
|||
| require.Len(t, resp.Beings, 3) | |||
|
|
|||
| // We should get the following three beings: | |||
There was a problem hiding this comment.
note to self: post-rebase, need to apply these changes to TestInterfaceListPointerField.
dnerdy
approved these changes
Aug 25, 2021
| // how to refactor. | ||
| for _, selection := range selectionSet { | ||
| field, ok := selection.(*ast.Field) | ||
| if !ok { // fragment/interface, not a shared field |
There was a problem hiding this comment.
As we discovered with the validator edge case, a union doesn't have shared fields, but it may be possible to treat the fields as shared if all the union members implement the same interface corresponding to a fragment. Anyway, this will be tackled in the upcoming fragment changes (if we want to tackle it at all).
There was a problem hiding this comment.
Ah, yeah that's an interesting situation, and maybe a place where abstract-in-abstract spreads are more useful. Anyway, this is gonna get rewritten as you say.
benjaminjkraft
force-pushed
the
benkraft.interfaces-3
branch
from
ef831b4 to
3f3c64d
Compare
benjaminjkraft
force-pushed
the
benkraft.interfaces-4
branch
from
4e8b2d7 to
e0eaca5
Compare
benjaminjkraft
force-pushed
the
benkraft.interfaces-3
branch
from
3f3c64d to
c37edb9
Compare
Right now, if you make a query like `{ myInterface { field } }`, you
have to type-switch on all the possible implementations of `myInterface`
to get at `field`. Now, we generate getter-methods (e.g. `GetField`),
to make that access easier. Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).
This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
`MyInterfaceMyFieldMyType`, not `MyInterfaceMyImplMyFieldMyType`, in the
case where you have an interface `MyInterface` implemented by `MyImpl`
(and maybe other types) with field `myField: MyType`. This is necessary
so the getter method returns a well-defined type, and also probably
convenient for calling code. It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.
While I was in the area, I added to the interface doc-comment a list of
the implementations. (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)
Issue: #8
Test plan:
make check
Reviewers: marksandstrom, adam, miguel
benjaminjkraft
force-pushed
the
benkraft.interfaces-4
branch
from
e0eaca5 to
b8b66f5
Compare
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:
Right now, if you make a query like
{ myInterface { field } }, youhave to type-switch on all the possible implementations of
myInterfaceto get at
field. Now, we generate getter-methods (e.g.GetField),to make that access easier. Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).
This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
MyInterfaceMyFieldMyType, notMyInterfaceMyImplMyFieldMyType, in thecase where you have an interface
MyInterfaceimplemented byMyImpl(and maybe other types) with field
myField: MyType. This is necessaryso the getter method returns a well-defined type, and also probably
convenient for calling code. It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.
While I was in the area, I added to the interface doc-comment a list of
the implementations. (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)
Issue: #8
Test plan:
make check