-
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
Expose "Group" Provide option #225
Conversation
Including an early (even if it's breaking) test case would be really beneficial to this review so people can see the intended usage and outcome |
4920903
to
2e831dd
Compare
Codecov Report
@@ Coverage Diff @@
## master #225 +/- ##
=========================================
- Coverage 98.64% 98.4% -0.24%
=========================================
Files 9 9
Lines 1104 1130 +26
=========================================
+ Hits 1089 1112 +23
- Misses 11 13 +2
- Partials 4 5 +1
Continue to review full report at Codecov.
|
2e831dd
to
947334a
Compare
@glibsm tests have been added :) |
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.
mostly lgtm.
couple testing nits and suggesting another test case.
} | ||
|
||
require.NoError(t, c.Invoke(func(i in) { | ||
assert.Equal(t, []int{2, 3, 1}, i.Values) |
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.
can avoid need for setting the rand seed by using assert.ElementsMatch
dig_test.go
Outdated
return out{Value: i} | ||
}, Group("val")), "This Provide should fail") | ||
} | ||
|
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.
function unnecessary here since this is called exactly once. consider inlining it.
require.Error(t, c.Provide(...))
The contents of the function can also mark the test as failed to guarantee that it's never called.
func() out {
t.Fatal("this function should never be called")
return out{}
}
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 may be subjective but inlining it breaks the format followed by surrounding tests and makes it a little harder to read.
325915f
to
893547b
Compare
The commits are mostly individually reviewable.
This is a step towards a solution for #610 (fx) for both named values and value groups. This will allow fx to expose its own Group option, which will allow users to form value groups without needing fx.Out.