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

Fixes #843: Bind to embedded struct method or field #919

Merged
merged 1 commit into from
Nov 11, 2019

Conversation

matiasanaya
Copy link
Contributor

@matiasanaya matiasanaya commented Nov 3, 2019

This is an attempt to write test cases for the buggy codegen.builder#findBindTarget method as initially raised in #843 .

Some of the issues:

  • It errors (and fails silently) if it finds an embedded exported interface within a struct.
  • It won't look into methods from un-exported embedded structs.
  • It can't bind to an interface.

I'm not entirely sure what's the expected behaviour in some of this cases since this docs, code comments here and examples (I'm certain I've seen an example on how to bind to an embedded interface) seem to contradict at times each other.

A way to fix some of these issues would be to use types.LookupFieldOrMethod. If there's some alignment on what the spec should be, I'm happy to update this PR with code that solves the issues.

The best way to run these tests is:

go generate ./codegen/testserver/embedded/... && go test ./codegen/testserver/embedded/... -count=1

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

coveralls commented Nov 3, 2019

Coverage Status

Coverage decreased (-0.1%) to 61.957% when pulling a745dc7 on matiasanaya:fix/find-bind-target into f80cab0 on 99designs:master.

@vektah
Copy link
Collaborator

vektah commented Nov 7, 2019

Looks good, PR description and title could use an update.

@matiasanaya matiasanaya changed the title Test cases for issue #843 Fixes #843: Bind to embedded structs method or field Nov 8, 2019
@matiasanaya matiasanaya changed the title Fixes #843: Bind to embedded structs method or field Fixes #843: Bind to embedded struct method or field Nov 8, 2019
@matiasanaya
Copy link
Contributor Author

matiasanaya commented Nov 8, 2019

@vektah thanks for the review. I've updated the PR title, fixed up the commits and cleaned up the unused code previously committed.

This PR fixes the following to issues:

  • findBindTarget fails to bind to embedded structs if it finds an embedded exported interface within a struct.
  • findBindTarget fails to bind to exported fields / methods of un-exported embedded structs.

I'm happy for this to be merged as is.

I'd love to do a followup PR to support embedded interfaces.

@matiasanaya matiasanaya mentioned this pull request Nov 10, 2019
2 tasks
@vektah vektah merged commit a745dc7 into 99designs:master Nov 11, 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.

3 participants