Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support interface unexported methods #169

Closed
wants to merge 1 commit into from

Conversation

dvic
Copy link
Contributor

@dvic dvic commented Jul 5, 2018

I like to use unexported methods with interfaces to create union types in Go. This PR fixes the "missing method" warning. It replaces the original calls to findMethod with findExportedMethod (which has the same logic).

@dvic dvic force-pushed the interface-unexported-methods branch from dc69352 to 455343c Compare July 5, 2018 13:07
@dvic
Copy link
Contributor Author

dvic commented Jul 5, 2018

Build was failing due to linting false positive, I had to add // nolint: megacheck to the violating line in the new example.

@vektah
Copy link
Collaborator

vektah commented Jul 6, 2018

I don't think there is enough here to justify a new example. Maybe just throw a test into codegen/ and move the models up into codegen/testdata?

something along the lines of https://github.com/vektah/gqlgen/blob/master/codegen/input_test.go#L35-L49?

@dvic dvic force-pushed the interface-unexported-methods branch 2 times, most recently from 64ec6b8 to 8c6d837 Compare July 28, 2018 11:38
@dvic
Copy link
Contributor Author

dvic commented Jul 28, 2018

@vektah sorry it took so long, I rebased and pushed again but this time without the example and with the testing approach you suggested. In order to properly test this I had to replace the fmt.FPrintf with stdErrLog.Printf, where stdErrLog is a logger instance from the standard library. Is this okay?

@dvic dvic force-pushed the interface-unexported-methods branch from 8c6d837 to 147ba3c Compare July 28, 2018 12:42
@dvic dvic force-pushed the interface-unexported-methods branch from 147ba3c to 47c26d0 Compare August 21, 2018 08:51
@dvic
Copy link
Contributor Author

dvic commented Aug 21, 2018

@vektah I updated the PR for 0.4.x. It seems that the interface types tests were removed? (while the test models were still defined). Not sure what you want to do with them, so for now I just added the tests back (with the updated codegen/gen path).

@vektah
Copy link
Collaborator

vektah commented Aug 23, 2018

All the tests that arent checking errors were moved into codegen/testserver those tests were taking way too long to run.

I added some notes on testing - https://github.com/99designs/gqlgen/blob/master/TESTING.md

@@ -41,3 +41,26 @@ func TestGenerateServer(t *testing.T) {
_, err = conf.Load()
require.NoError(t, err)
}

func generate(name string, schema string, typemap ...TypeMap) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this moving?

Copy link
Collaborator

@vektah vektah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets get this in, I think its if the interface has a private method it needs to be present for the generated code to be valid.

Little bit of cleanup to do, I don't think hooking into the log is great, I wonder if this should just be an error?

"github.com/stretchr/testify/require"
)

func TestShapes(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Foo": {Model: "github.com/99designs/gqlgen/codegen/testserver.IfaceFoo"},
})
require.NoError(t, err)
assert.Equal(t, "", buf.String(), "using an interface with unexported methods should not emit warnings")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than test for the absence of a warning, could you look to see if a resolver was generated?

eg https://github.com/99designs/gqlgen/blob/master/codegen/testserver/generated_test.go#L24-L28

@vektah
Copy link
Collaborator

vektah commented Oct 2, 2018

This change has been rolled into #335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants