-
Notifications
You must be signed in to change notification settings - Fork 683
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
Remove snowball.Initialize
and snowball.Factory
#2104
Conversation
f := &Flat{ | ||
params: params, | ||
} | ||
f.nnarySnowball.Initialize(params.BetaVirtuous, params.BetaRogue, choice) |
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.
q: why not dropping this Initialize as well?
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 is being done in a followup
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.
Want to see if we can get rid of the function passing in the consensus test
@@ -16,16 +16,18 @@ var ( | |||
_ Consensus = (*Byzantine)(nil) | |||
) | |||
|
|||
func NewByzantine(_ Parameters, choice ids.ID) Consensus { |
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 we get rid of the unused parameter here? Looks like we only had it previous for interface signature
@@ -26,34 +28,44 @@ func (n *Network) Initialize(params Parameters, numColors int) { | |||
} | |||
} | |||
|
|||
func (n *Network) AddNode(sb Consensus) { | |||
func (n *Network) AddNode(newConsensusFunc newConsensusFunc) Consensus { |
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.
Is it possible for the caller to just give us an initialized Consensus
instance instead of this function result?
@@ -28,12 +28,12 @@ func TestSnowballOptimized(t *testing.T) { | |||
|
|||
sampler.Seed(seed) | |||
for i := 0; i < numNodes; i++ { | |||
nBitwise.AddNode(&Tree{}) | |||
nBitwise.AddNode(NewTree) |
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'm wondering if it's possible for us to pass parameters into NewTree
and NewFlat
so we can avoid passing in this function directly
Why this should be merged
Down with
Initialize
methods.How this works
Remove
snowball.Initialize
andsnowball.Factory
.How this was tested
Existing UT.