Add SkipAcyclicVerification Option, and container.VerifyAcyclic #209
Conversation
dig.go
Outdated
| @@ -211,6 +217,20 @@ func New(opts ...Option) *Container { | |||
| return c | |||
| } | |||
|
|
|||
| // DeferAcyclicVerification is an Option to override the default behavior | |||
| // of container.Provide, disabling the validation the dependency graph remains | |||
There was a problem hiding this comment.
nit: this comment needs updating. Otherwise looks good to me. I didn't thoroughly verify the correctness of your code, I'll leave that to the folks who actually own dig and know the internals better.
There was a problem hiding this comment.
I actually did rewrite this a bit. Is your objection to the word "disabling"? I think my sentence is confusing. This one combined with the last sentence in the comment block is intended to indicate the container will no longer validate on each Provide, but it will before the first Invoke.
…cycle detection error
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 9 9
Lines 1010 1034 +24
==========================================
+ Hits 998 1022 +24
Misses 10 10
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 9 9
Lines 1010 1033 +23
==========================================
+ Hits 998 1021 +23
Misses 10 10
Partials 2 2
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 9 9
Lines 1010 1033 +23
==========================================
+ Hits 998 1021 +23
Misses 10 10
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 9 9
Lines 1010 1033 +23
==========================================
+ Hits 998 1021 +23
Misses 10 10
Partials 2 2
Continue to review full report at Codecov.
|
3 similar comments
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 9 9
Lines 1010 1033 +23
==========================================
+ Hits 998 1021 +23
Misses 10 10
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 9 9
Lines 1010 1033 +23
==========================================
+ Hits 998 1021 +23
Misses 10 10
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 9 9
Lines 1010 1033 +23
==========================================
+ Hits 998 1021 +23
Misses 10 10
Partials 2 2
Continue to review full report at Codecov.
|
dig.go
Outdated
| } | ||
| } | ||
|
|
||
| c.isVerifiedAcyclic = true |
There was a problem hiding this comment.
nit: I would set this in the if clause above where you're checking it so
that it reads like this:
if !isVerified {
verify()
isVerified = true
}
There was a problem hiding this comment.
I'm not sure I agree. The if clause above is only testing starting from a single node. We don't really know that the container is verifiably acyclic until after all nodes have been tested.
There was a problem hiding this comment.
Oh, my comment may have been confusing. I was talking about the one in Invoke.
if !c.isVerifiedAcyclic {
if err := c.verifyAcyclic(); err != nil {
return err
}
}
Right now, we check isVerifiedAcyclic in Invoke, then verifyAcylic sets it.
I'm suggesting,
if !c.isVerifiedAcyclic {
if err := c.verifyAcyclic(); err != nil {
return err
}
c.isVerifiedAcyclic = true
}
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 9 9
Lines 1010 1033 +23
==========================================
+ Hits 998 1021 +23
Misses 10 10
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
3 similar comments
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
dnathe4th
changed the title
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 98.81% 98.93% +0.12%
==========================================
Files 9 9
Lines 1010 1032 +22
==========================================
+ Hits 998 1021 +23
+ Misses 10 9 -1
Partials 2 2
Continue to review full report at Codecov.
|
This adds a DeferAcyclicVerification option which defers cycle detection until the first invoke.
It is not unreasonable for libraries using
digunder the hood for dependency graph management toc.Provideconstructors in a tight loop. Rather than paying the cost of of verifying the graph is acyclic for everyProvide, this option and method combination can be used to verify the graph once, after the loop has completed.