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

refactor!: remove builder return param from init #2165

Merged
merged 6 commits into from
Jul 27, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jul 25, 2023

Closes #2157

A separate Init() function seems kinda like an anti pattern. If it must always be called after NewBuilder, why doesn't NewBuilder call it? @mojtaba-esk

@rootulp rootulp requested a review from mojtaba-esk July 25, 2023 18:00
@rootulp rootulp self-assigned this Jul 25, 2023
@rootulp rootulp added the refactor optional label for items that are related to implementation work and do not change functionality label Jul 25, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2165 (0eea0e5) into main (3fc2fed) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 0eea0e5 differs from pull request most recent head 16fbdfe. Consider uploading reports for the commit 16fbdfe to get more accurate results

@@            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     
Files Changed Coverage Δ
pkg/shares/padding.go 53.06% <100.00%> (+0.97%) ⬆️
pkg/shares/share_builder.go 79.59% <100.00%> (+3.27%) ⬆️
pkg/shares/split_compact_shares.go 67.16% <100.00%> (+0.75%) ⬆️
pkg/shares/split_sparse_shares.go 47.82% <100.00%> (+1.55%) ⬆️

@rootulp rootulp marked this pull request as ready for review July 25, 2023 18:30
evan-forbes
evan-forbes previously approved these changes Jul 26, 2023
pkg/shares/share_builder.go Outdated Show resolved Hide resolved
@mojtaba-esk
Copy link
Member

A separate Init() function seems kinda like an anti pattern. If it must always be called after NewBuilder, why doesn't NewBuilder call it? @mojtaba-esk

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.

@evan-forbes ^

Copy link
Member

@mojtaba-esk mojtaba-esk left a 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

mojtaba-esk
mojtaba-esk previously approved these changes Jul 26, 2023
pkg/shares/share_builder.go Outdated Show resolved Hide resolved
@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

smart idea

@MSevey MSevey requested a review from a team July 26, 2023 14:34
@rootulp rootulp enabled auto-merge (squash) July 26, 2023 14:35
Copy link
Contributor

@cmwaters cmwaters left a 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

@rootulp rootulp merged commit 4762f4d into celestiaorg:main Jul 27, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor optional label for items that are related to implementation work and do not change functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

review: Builder Init - consider not returning builder
5 participants