Implementing a dig.As ProvideOption #233

alessandrozucca · 2019-03-07T20:30:36Z

This adds a dig.As option which allows the caller to specify a
constructor and a list of interfaces that the constructed struct
implements.

Closes #197

CLAassistant · 2019-03-07T20:30:45Z

All committers have signed the CLA.

alessandrozucca · 2019-03-07T20:31:27Z

still need to add a test for the DotResult()..

codecov · 2019-03-07T20:32:45Z

Codecov Report

Merging #233 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage    98.4%   98.47%   +0.06%     
==========================================
  Files          10       10              
  Lines        1130     1179      +49     
==========================================
+ Hits         1112     1161      +49     
  Misses         13       13              
  Partials        5        5

Impacted Files	Coverage Δ
result.go	`98.31% <100%> (+0.22%)`	⬆️
dig.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a28c4b...2702501. Read the comment docs.

amckinney

Thanks for the contribution! Looks good overall - I only have a minor suggestion with respect to the implementation. Can I get another pair of eyes, @akshayjshah @glibsm?

dig.go

amckinney

We can arguably mimic this behavior in the current design by explicitly providing constructors for the interfaces you want to provide:

type Foo struct {}

func (f Foo) String() string {
  return "foo"
}

...

Provide(func () Foo { return Foo{} })
Provide(func (f Foo) fmt.Stringer { return f })

Regardless, the As semantic is nice so I think it's something to consider. I'm curious to hear others' thoughts here.

dig.go

alessandrozucca · 2019-03-12T20:40:04Z

We can arguably mimic this behavior in the current design by explicitly providing constructors for the interfaces you want to provide

Yeah that was the suggestion on the issue too. The problem is that (at iZettle) we opted for explicitly declare the function providers that you've put in one line.

This means that some of our fx modules where we do the DI wiring are quite large. Having the As(..) makes the code much nicer and decreases the amount of functions we need to declare.

This adds a dig.As option which allows the caller to specify a constructor and a list of interfaces that the constructed struct implements. Closes uber-go#197

alessandrozucca · 2019-03-19T20:16:16Z

@amckinney, @glibsm, @prashantv

I have applied the changes requested and is ready for a re-review

This rewords the error messages for `dig.As` validation to specify the type encountered rather than the Kind.

The dig.As functionality shouldn't introduce its own result type because this isn't a new leaf kind. It's another option for the case when a single result is being provided. This change merges resultSingle and resultAs. In the process, the conversion of the dig.As arguments to reflect.Types is done once and re-used throughout.

abhinav · 2019-03-21T20:53:59Z

Thanks for the change @alessandrozucca! Besides minor nits around the error
messages, an implementation level concern I had here was that resultAs didn't
need to be its own result type. The duplication between resultSingle and
resultAs is indicative of this. To me, dig.As and the corresponding behavior
is another option for resultSingle.

I went ahead and made the required changes to avoid a bunch of back-and-forth.
Feel free to take a look.

CC @amckinney @glibsm @prashantv

alessandrozucca · 2019-03-22T08:03:43Z

@abhinav thank you, looks better now

It did cross my mind, didn't do it for two reasons:

I wanted just to add code (leaving existing code as it is without refactoring)
the name resultSingle made me think that had to provide only 1 result, adding the As() option it will provide multiple results: one as the concrete struct and one for each interface

Having said that, reusing resultSingle now makes sense to me 👍

amckinney · 2019-03-22T20:26:07Z

dig.go

@@ -127,6 +151,28 @@ func Group(group string) ProvideOption {
 	})
 }

+// As is a ProvideOption that specifies that the struct produced by the constructor
+// implements the given interface


Duplicate doc.

Derp. Forgot to delete.

amckinney · 2019-03-22T20:26:46Z

dig.go

@@ -598,9 +659,10 @@ type node struct {

 type nodeOptions struct {
 	// If specified, all values produced by this node have the provided name
-	// or belong to the specified value group
+	// belong to the specified value group or implement any of the interfaces


nit: Missing period.

This adds a test for one of the missing invalid cases and verifies that dig.As plays nicely with graph visualization.

abhinav · 2019-03-25T21:29:35Z

Added a visualization test. The output looks like this:

prashantv

Change looks good to me -- a couple of additional unit tests would be good, expectially for As and Named being combined.

Is it clear that the Name applies to the As as well? E.g.,

c.Provide(NewBuffer, dig.Name("foo"), dig.As(new(io.Writer)))

It's not clear to me whether the writer is also named, or whether we can do dig.As(new(io.Writer), dig.Name("foo")). If we're not sure, we could disallow it too.

prashantv · 2019-03-25T23:00:21Z

dig.go

 	case resultGrouped:
 		// we don't really care about the path for this since conflicts are
 		// okay for group results. We'll track it for the sake of having a
 		// value there.
 		k := key{group: r.Group, t: r.Type}
 		cv.keyPaths[k] = path
+


nit: can remove blank

prashantv · 2019-03-25T23:03:26Z

dig_test.go

@@ -1905,6 +2024,46 @@ func TestProvideFailures(t *testing.T) {
 			`dig.out embeds \*dig.Out`,
 		)
 	})
+


couple of additional tests:

use Named and As together (and verify the behaviour -- ensure both the function's return type and the As type are added to the container with the name).

provide a function that returns an interface, and then use As. If the As is for the same type, it should fail, but it should work if it's a different interface type

This adds a test to verify that dig.As is compatible with dig.Name and updates the documentation to clarify this.

If dig.As is called with the same type the constructor is producing, ignore that As entry rather than erroring out.

abhinav · 2019-03-27T00:36:48Z

Thanks for the contribution @alessandrozucca! We'll try to release this soon.

This reverts #233 until we resolve discussion on it so that we don't accidentally release it. Reopens #197

This brings back #233. Per #235 (review), the issues we need to resolve are, 1. `dig.As` seems to indicate that it's a total override of the provided type. The way it currently works is it appends other interfaces on top of whatever the constructor already returns 2. semantics of `dig.Provide(func New() (Foo, io.Reader, error), dig.As(new(io.Writer)`. It currently fails due to inability to case reader to writer, but we'd want some extra validation here. Perhaps `dig.As` is only supported for a single type, error tuple? Closes #197

This reverts #233 until we resolve discussion on it so that we don't accidentally release it. Reopens #197

This brings back #233. Per #235 (review), the issues we need to resolve are, 1. `dig.As` seems to indicate that it's a total override of the provided type. The way it currently works is it appends other interfaces on top of whatever the constructor already returns 2. semantics of `dig.Provide(func New() (Foo, io.Reader, error), dig.As(new(io.Writer)`. It currently fails due to inability to case reader to writer, but we'd want some extra validation here. Perhaps `dig.As` is only supported for a single type, error tuple? Closes #197

This sets up release v1.8.0. The only change in this release is the migration to Go modules. We backed out #233; we'll try it again in #252.

This brings back uber-go#233. Per uber-go#235 (review), the issues we need to resolve are, 1. `dig.As` seems to indicate that it's a total override of the provided type. The way it currently works is it appends other interfaces on top of whatever the constructor already returns 2. semantics of `dig.Provide(func New() (Foo, io.Reader, error), dig.As(new(io.Writer)`. It currently fails due to inability to case reader to writer, but we'd want some extra validation here. Perhaps `dig.As` is only supported for a single type, error tuple? Closes uber-go#197

This brings back Dig.As from #233 with some changes. Specifically, Dig.Provide(t1, As(t2))used to provide type t2 as well as type t1 but that is no longer the case. For example, c.Provide(newBuffer, dig.As(new(io.Reader), new(io.Writer))) will provide io.Reader and io.Writer, but not buffer. Refs: GO-451

amckinney approved these changes Mar 12, 2019

View reviewed changes

amckinney reviewed Mar 12, 2019

View reviewed changes

dig.go Outdated Show resolved Hide resolved

amckinney reviewed Mar 12, 2019

View reviewed changes

dig.go Outdated Show resolved Hide resolved

alessandrozucca force-pushed the dig-as branch from 97d8755 to 3f2c360 Compare March 13, 2019 17:19

Implementing a dig.As ProvideOption

900c2a9

This adds a dig.As option which allows the caller to specify a constructor and a list of interfaces that the constructed struct implements. Closes uber-go#197

alessandrozucca force-pushed the dig-as branch from 3f2c360 to 900c2a9 Compare March 15, 2019 17:05

abhinav added 2 commits March 21, 2019 13:50

as: Reword error message

6fbdc8e

This rewords the error messages for `dig.As` validation to specify the type encountered rather than the Kind.

amckinney approved these changes Mar 22, 2019

View reviewed changes

dig.As: Add more tests

302b75e

This adds a test for one of the missing invalid cases and verifies that dig.As plays nicely with graph visualization.

dig.go: Delete redundant comment and fix grammar

234f709

prashantv reviewed Mar 25, 2019

View reviewed changes

abhinav added 3 commits March 26, 2019 17:01

As: Ensure compatibility with Name

30172a7

This adds a test to verify that dig.As is compatible with dig.Name and updates the documentation to clarify this.

As: Ignore same type

c1ad1e5

If dig.As is called with the same type the constructor is producing, ignore that As entry rather than erroring out.

As/test: Verify conversion to other interfaces

2702501

abhinav merged commit 4cc9f88 into uber-go:master Mar 27, 2019

abhinav added a commit that referenced this pull request Mar 27, 2019

CHANGELOG: Entry for #233

b52dd74

glibsm mentioned this pull request Sep 30, 2019

Add API to invoke named values #181

Open

abhinav added a commit that referenced this pull request Nov 14, 2019

Revert "Implement a dig.As ProvideOption (#233)"

588ff58

This reverts #233 until we resolve discussion on it so that we don't accidentally release it. Reopens #197

abhinav mentioned this pull request Nov 14, 2019

Revert "Implement a dig.As ProvideOption (#233)" #251

Merged

abhinav mentioned this pull request Nov 14, 2019

Implement a dig.As ProvideOption #252

Closed

abhinav added a commit that referenced this pull request Nov 14, 2019

Revert "Implement a dig.As ProvideOption (#233)" (#251)

4fb70ce

This reverts #233 until we resolve discussion on it so that we don't accidentally release it. Reopens #197

abhinav added a commit that referenced this pull request Nov 14, 2019

Preparing release v1.8.0

6a980b3

This sets up release v1.8.0. The only change in this release is the migration to Go modules. We backed out #233; we'll try it again in #252.

abhinav mentioned this pull request Nov 14, 2019

Preparing release v1.8.0 #253

Merged

abhinav added a commit that referenced this pull request Nov 14, 2019

Preparing release v1.8.0 (#253)

be30405

This sets up release v1.8.0. The only change in this release is the migration to Go modules. We backed out #233; we'll try it again in #252.

sywhang mentioned this pull request Sep 15, 2021

Add Dig.As ProvideOption #293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing a dig.As ProvideOption #233

Implementing a dig.As ProvideOption #233

alessandrozucca commented Mar 7, 2019

CLAassistant commented Mar 7, 2019 •

edited

Loading

alessandrozucca commented Mar 7, 2019

codecov bot commented Mar 7, 2019 •

edited

Loading

amckinney left a comment

amckinney left a comment

alessandrozucca commented Mar 12, 2019 •

edited

Loading

alessandrozucca commented Mar 19, 2019

abhinav commented Mar 21, 2019

alessandrozucca commented Mar 22, 2019 •

edited

Loading

amckinney Mar 22, 2019

abhinav Mar 25, 2019 •

edited

Loading

amckinney Mar 22, 2019

abhinav commented Mar 25, 2019

prashantv left a comment

prashantv Mar 25, 2019

prashantv Mar 25, 2019

abhinav commented Mar 27, 2019

Implementing a dig.As ProvideOption #233

Implementing a dig.As ProvideOption #233

Conversation

alessandrozucca commented Mar 7, 2019

CLAassistant commented Mar 7, 2019 • edited Loading

alessandrozucca commented Mar 7, 2019

codecov bot commented Mar 7, 2019 • edited Loading

Codecov Report

amckinney left a comment

Choose a reason for hiding this comment

amckinney left a comment

Choose a reason for hiding this comment

alessandrozucca commented Mar 12, 2019 • edited Loading

alessandrozucca commented Mar 19, 2019

abhinav commented Mar 21, 2019

alessandrozucca commented Mar 22, 2019 • edited Loading

amckinney Mar 22, 2019

Choose a reason for hiding this comment

abhinav Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

amckinney Mar 22, 2019

Choose a reason for hiding this comment

abhinav commented Mar 25, 2019

prashantv left a comment

Choose a reason for hiding this comment

prashantv Mar 25, 2019

Choose a reason for hiding this comment

prashantv Mar 25, 2019

Choose a reason for hiding this comment

abhinav commented Mar 27, 2019

CLAassistant commented Mar 7, 2019 •

edited

Loading

codecov bot commented Mar 7, 2019 •

edited

Loading

alessandrozucca commented Mar 12, 2019 •

edited

Loading

alessandrozucca commented Mar 22, 2019 •

edited

Loading

abhinav Mar 25, 2019 •

edited

Loading