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 for embedded struct type in resolver #338

Merged
merged 8 commits into from
Sep 17, 2019
Merged

Support for embedded struct type in resolver #338

merged 8 commits into from
Sep 17, 2019

Conversation

eloyekunle
Copy link
Contributor

Fixes #331

Updated example in ./example/social/social.go to demonstrate how it works.

Follow up to: #282
Ref: #28

@pavelnikolov
Copy link
Member

What happens if there is more than one embedded struct with the same field?
This feature has been requested in the past by many people. There is one problem with it, though. Imagine we have more than one embedded struct containing fields with the same name. Then you'd end up with ambiguous code. For example: https://play.golang.org/p/0gvABy9DZ5Y
IMHO, this feature should not be merged because it creates edge cases and possible ambiguities.

@eloyekunle
Copy link
Contributor Author

eloyekunle commented Jul 1, 2019

@pavelnikolov I have added a method, getFieldCount that helps with checking for ambiguities so embedded structs containing fields with the same name will throw an error.

@eloyekunle
Copy link
Contributor Author

@pavelnikolov Please take another look.

for _, f := range fields {
fieldIndex := -1
var fieldIndex []int
Copy link
Member

Choose a reason for hiding this comment

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

Why does the field index is an array? I don't get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an array because it is an index sequence for Type.FieldByIndex.

It allows for several layers of anonymously-nested fields.

@@ -380,13 +384,46 @@ func findMethod(t reflect.Type, name string) int {
return -1
}

func findField(t reflect.Type, name string) int {
func findField(t reflect.Type, name string, index []int) []int {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this method need an integer array? , index []int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function builds the index sequence for Type.FieldByIndex recursively, returning the sequence if found and an empty array otherwise.

@eloyekunle
Copy link
Contributor Author

eloyekunle commented Sep 13, 2019

I've also added a unit test for the feature here.

Name string // ambiguous
}

func TestPanicAmbiguity(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you, please, move ambiguousResolver and University inside of the test (e.i. TestPanicAmbiguity)?

type Timestamps struct {
CreatedAt string
UpdatedAt string
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Can you, please, move the definition of these structs inside of the test TestEmbeddedStruct?

Copy link
Member

@pavelnikolov pavelnikolov left a comment

Choose a reason for hiding this comment

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

Please, move the struct definitions inside of the corresponding tests and I'll be happy to merge this PR.

@pavelnikolov
Copy link
Member

Actually, I'll leave it like this for consistency. Refactoring should be in a separate PR.

@pavelnikolov pavelnikolov merged commit 38a077b into graph-gophers:master Sep 17, 2019
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.

Support for embedded struct type in resolver
2 participants