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

Adding capability to auto-bind a type to interfaces it implements #285

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dabbertorres
Copy link

@dabbertorres dabbertorres commented Apr 22, 2021

Implements #242.

I think motivation for this was covered well by @bradleypeabody in the linked issue, so I won't go into detail here, but I'll copy over a statement I liked:

The Go language itself resolves interfaces based solely on method signatures, not an explicit implements mechanism - I don't see why wire wouldn't follow the same behavior. wire.Bind can still be used to resolve ambiguous cases.

Summary

Public API

  • Addition of wire.AutoBind() and corresponding AutoBinding type.

Internals

  • Addition of a new AutoBinding type to the internal/wire package that records a concrete type and position of its token.
  • AutoBinding fields added to the ProviderSet, providerSetSrc, and ProviderType structs.
  • Check for fully unused AutoBindings.
  • processAutoBind() to handle the parsing
  • The code to actually implement the auto-binding behavior was added to (*ProviderSet).For().
    This makes it easy to give explicit wire.Bind()s precedence. If a type is not found in the providerMap, and it is an interface, the []AutoBindings list is checked for a type that implements the interface. If one is found, it is added to the providerMap and srcMap to avoid future searches for the same type. If no such type is found, behavior is as before.

Tests

  • A simpler InterfaceAutoBinding showing automated binding of Bar -> Fooer.
  • A more complex InterfaceExplicitBindingPreferredOverAutoBinding, which is fairly self-descriptive.

I've tried out my fork on a project that was already using wire, and so far it is working rather nicely. It has made the wire.Build()s a bit nicer I think - especially for cases where one concrete type implements 2 or more interfaces.
Coupling is reduced too, since the implementation and the interface don't need to directly know about each other. Usage seems more "Go-ish" from my (obviously a bit biased) point of view.

Any feedback/comments/concerns is appreciated! This was my first time reading through wire's code (though I've used it a lot), so I'll be happy to make any changes necessary if there are concerns around the structure of the implementation.

@google-cla google-cla bot added the cla: yes Google CLA has been signed! label Apr 22, 2021
@zombiezen
Copy link
Collaborator

Hey @dabbertorres, thanks for the PR! I'm a bit crunched for time for the next month, so I'm not sure when I will have time to review, but rest assured that this is on my radar.

@vangent or @shantuo, if you have more bandwidth to review, feel free. I'm okay with the direction of this PR (see the discussion on #242), but I haven't looked into any of the specifics.

@blank-teer blank-teer mentioned this pull request Aug 18, 2021
@eliben
Copy link
Member

eliben commented Aug 19, 2021

I don't think we're going to accept this PR for now. Nothing wrong with the PR itself, but wire is in maintenance mode. It does what we need it to do for go-cloud, and works well for other users as well. Being slightly more explicit than necessary at times seems like a reasonable tradeoff for us, valuing simplicity.

I'll keep the PR open for now so interested users can find and apply it if they really need this functionality.

@dabbertorres dabbertorres force-pushed the autobinding-experiment branch 3 times, most recently from 953c8ab to 82059a6 Compare August 21, 2021 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA has been signed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants