-
Notifications
You must be signed in to change notification settings - Fork 286
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
refactor!: remove builder return param from init #2165
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2165 +/- ##
==========================================
+ Coverage 24.41% 24.45% +0.04%
==========================================
Files 126 126
Lines 14300 14301 +1
==========================================
+ Hits 3491 3498 +7
+ Misses 10448 10444 -4
+ Partials 361 359 -2
|
Don't remember the rational behind it, I checked the code and probably the reason does not exist any more. So we can merge the two functions. |
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.
LGTM, please go on with the proposal to merge the Init into the NewBuilder
@@ -318,3 +311,10 @@ func TestShareBuilderImportRawData(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
// mustNewBuilder returns a new builder with the given parameters. It fails the test if an error is encountered. | |||
func mustNewBuilder(t *testing.T, ns appns.Namespace, shareVersion uint8, isFirstShare bool) *Builder { |
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.
smart idea
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.
Yeah this makes sense to me
Closes #2157
A separate
Init()
function seems kinda like an anti pattern. If it must always be called afterNewBuilder
, why doesn'tNewBuilder
call it? @mojtaba-esk