-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
constructor: allow nil options #603
Conversation
var optsRun []int | ||
optcount := 0 | ||
newOpt := func() Option { | ||
index := optcount |
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.
nit: possibly remove the index assignment by incrementing optcount in a defer statement?
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.
Sure.
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.
Ah, wait, no. We need to capture the current value.
This can make it significantly easier to configure libp2p with optional options.
13cd715
to
cff9cb5
Compare
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.
generally lgtm, just some small feedback
if err := cfg.Apply(newOpt(), nil, ChainOptions(newOpt(), newOpt(), ChainOptions(), ChainOptions(nil, newOpt()))); err != nil { | ||
t.Fatal(err) | ||
} | ||
if optcount != len(optsRun) { |
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.
let's compare against a constant here, i.e. 4
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.
SGTM.
if optcount != len(optsRun) { | ||
t.Errorf("expected to have handled %d options, handled %d", optcount, len(optsRun)) | ||
} | ||
for i, x := range optsRun { |
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.
if this is meant to test ordering, definitely drop a comment so future programmers understand the behavior
This can make it significantly easier to configure libp2p with optional options.