-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
silencing the SDK to ensure it does not panic #4314
Conversation
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.
🎉 Thanks for picking this up! Would you mind adding a test to assert that the empty config works OK now where it panicked before?
sdk/options.go
Outdated
bs, err := ioutil.ReadAll(o.Config) | ||
if err != nil { | ||
return err | ||
var bs = []byte("{}") |
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] This should be the same as
var bs = []byte("{}") | |
bs := []byte("{}") |
where I think the bs :=
variant is a bit more consistent with the overall code style
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.
Made the recommended adjustments and added a test case.
sdk/options.go
Outdated
var bs = []byte("{}") | ||
|
||
if o.Config == nil { | ||
o.config = bs |
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.
Also a nit: I think we could inline the value of bs
here, as there is really no need for it in the outer scope: o.config = []byte("{}")
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 actually like this one better. Making that adjustment now
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, one final nitpick
If you wouldn't mind, could you please also squash the commits? A short message including
would be great, too. |
Signed-off-by: Damien Burks <[email protected]>
f99e916
to
a01c864
Compare
@srenatus Everything looks good to me. Please check and confirm when you get a chance. |
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. Thanks!
Addresses issue #4303