-
Notifications
You must be signed in to change notification settings - Fork 232
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
Disallow GetPublicKey when DisableValues is passed #604
Conversation
dht_options.go
Outdated
|
||
if !c.enableValues { | ||
c.validator = nil | ||
} |
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.
@Stebalien do you think this is worth doing or no? We have some tests and I've done a once-over and it looks like we shouldn't have any problems with nil
validators if enableValues=false
, if it's enableValues=true
though is it fair to say the developer has shot themselves in the foot or should we throw in a bunch of nil
checks or implement a NilValidator
that's applied here in the fallbacks.
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.
Why not just leave the validator?
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 guess no particular reason, it's not like it's going to eat up much memory. This was more a point for discussing what we should do around nil
validators.
I'll remove those few lines and merge.
dht_options.go
Outdated
|
||
if !c.enableValues { | ||
c.validator = nil | ||
} |
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.
Why not just leave the validator?
57ffb53
to
b95fd5d
Compare
…ic keys (i.e. the /pk namespace)
b95fd5d
to
d5d5a9e
Compare
This is missing a test to confirm the new behavior, but is otherwise ready |
Let's not leave it in a PR. |
No description provided.