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

Call variadic constructors without arguments #123

Merged
merged 3 commits into from
Jul 11, 2017

Conversation

abhinav
Copy link
Collaborator

@abhinav abhinav commented Jul 11, 2017

Dig constructors may now be variadic functions. These functions will be
called with their dependencies as usual and no arguments will be passed
for the variadic arguments.

So a constructor New(*Foo, *Bar, ...Option) *Baz will be treated as
New(*Foo, *Bar) *Baz.

See #120 for more discussion.

Dig constructors may now be variadic functions. These functions will be
called with their dependencies as usual and no arguments will be passed
for the variadic arguments.

So a constructor `New(*Foo, *Bar, ...Option) *Baz` will be treated as
`New(*Foo, *Bar) *Baz`.

See #120 for more discussion.
@abhinav abhinav requested review from glibsm, akshayjshah and a user July 11, 2017 18:27
@codecov
Copy link

codecov bot commented Jul 11, 2017

Codecov Report

Merging #123 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
+ Coverage    99.6%   99.61%   +0.01%     
==========================================
  Files           3        3              
  Lines         254      263       +9     
==========================================
+ Hits          253      262       +9     
  Misses          1        1
Impacted Files Coverage Δ
dig.go 99.54% <100%> (+0.01%) ⬆️

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 d6bf8ca...1569dcc. Read the comment docs.

dig.go Outdated
@@ -389,8 +396,15 @@ func newNode(k key, ctor interface{}, ctype reflect.Type) (*node, error) {

// Retrieves the dependencies for a constructor
func getConstructorDependencies(ctype reflect.Type) ([]edge, error) {
numArgs := ctype.NumIn()
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets extract into a function since the code and comment appears twice

Copy link
Collaborator

@glibsm glibsm left a comment

Choose a reason for hiding this comment

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

lgtm, but lets get a signoff from some folk who were more active on #120 than me.


require.NoError(t, c.Invoke(func(a *A, as ...*A) {
require.NotNil(t, a, "A must not be nil")
require.True(t, a == gaveA, "A must match")
Copy link

Choose a reason for hiding this comment

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

Curious, why not require.Equal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

require.Equal on pointers dereferences and compares value. Because this is an empty type (struct{}), that will always be true. == on pointers compares the pointer value to verify that it's the same instance of the object.

}), "failed to provide A")

require.NoError(t, c.Provide(func() []*A {
panic("[]*A constructor must not be called.")
Copy link

Choose a reason for hiding this comment

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

require.FailNow()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was sort of trying to stay consistent with existing tests that use panic.

Plus with require.FailNow, you now have to have an empty return statement to satisfy the compiler.

I can change this if you feel strongly about this.

Copy link

Choose a reason for hiding this comment

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

Not really, panic is fine too.

Copy link
Contributor

@HelloGrayson HelloGrayson left a comment

Choose a reason for hiding this comment

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

cool

@abhinav abhinav merged commit a730197 into master Jul 11, 2017
@abhinav abhinav deleted the abhinav/variadic-disallow branch July 11, 2017 21:50
hbdf pushed a commit to hbdf/dig that referenced this pull request Jul 14, 2022
hbdf pushed a commit to hbdf/dig that referenced this pull request Jul 14, 2022
* Only install dependencies once for examples

Fixes uber-go#118
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants