-
Notifications
You must be signed in to change notification settings - Fork 206
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
Add Invoke function to resolve dependency #23
Conversation
container.go
Outdated
// occurred during the execution | ||
func (c *Container) Invoke(t interface{}) error { | ||
c.Lock() | ||
defer c.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if locking is needed here since we are not updating the container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets move unlock to right before Call.
still need to lock for map read though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to RLock()
container_test.go
Outdated
var c1 Child1 | ||
var c2 Child2 | ||
|
||
err = c.Invoke(func(p1 *Parent1, p2 *Parent12) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate p1 and p2
@@ -112,6 +145,8 @@ func (c *Container) Resolve(obj interface{}) (err error) { | |||
objElemType := reflect.TypeOf(obj).Elem() | |||
objVal := reflect.ValueOf(obj) | |||
|
|||
c.Lock() | |||
defer c.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might also be good to just put the RLock around the map access, but not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
I'm not sure that this fully satisfies the requirement we have for That can probably be an option passed into |
I added the functionality to register returned objects back to the graph. This satisfies the requirement of executing Invoke function and having the returned objects available for later Resolve. The Invoke will also support anonymous functions, if users don't wish to register any objects back to dig. |
container.go
Outdated
} | ||
args[idx] = v | ||
} else { | ||
return fmt.Errorf("%v dependency of type %v is not registered", ctype, arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should move this to c.get(type)
so this is not duplicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should also do errors.Wrapf like the one above
return nil | ||
} | ||
|
||
func (c *Container) insertObjectToGraph(v reflect.Value, vtype reflect.Type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no lock in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lock is applied in Provide
and Invoke
around insertObject func. I am refactoring this to move locking out and away from the container to simplify the API.
forgot to mention, could you update changelog? |
* Exclude nil providers from ProviderGroup * Removed comment * Capture oldProviders
#21
Invoke function resolves the dependencies provided as arguments to the constructor, and invokes the function. The Invoke(func) can be used as a callback function with resolved dependencies from dig.