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

Support slices in event.GetObjValue and add tests #506

Merged
merged 2 commits into from
Jan 8, 2018

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Jan 5, 2018

Modifies event.GetObjValue so that when specifying additional keys to
descend into, it's now possible to specify integer keys if the target
object is a slice of type []interface{} (which should be what anything
coming out of event data deserializes to).

I initially expressed some concern about expanding this interface, but
given that the previous behavior would have been an outright panic, I
changed my mind and don't think it's too bad to add support for this
given that it's been requested by a user.

Fixes #503.

r? @ob-stripe
cc @stripe/api-libraries

Modifies `event.GetObjValue` so that when specifying additional keys to
descend into, it's now possible to specify integer keys if the target
object is a slice of type `[]interface{}` (which should be what anything
coming out of event data deserializes to).

I initially expressed some concern about expanding this interface, but
given that the previous behavior would have been an outright panic, I
changed my mind and don't think it's too bad to add support for this
given that it's been requested by a user.

Fixes #503.
event_test.go Outdated
// cases.
assert.Panicsf(t, func() {
event.GetObjValue("slice", "string_key")
}, "Cannot access nested slice element with non-integer key: %s", "bad_key")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm likely missing something since the tests are passing, but shouldn't the assert expect "string_key", not "bad_key"?

@ob-stripe
Copy link
Contributor

I left a question in a comment, but it's likely just me missing something. lgtm!

(On an unrelated note, it looks like tests are failing on go-tip.)

@stripe-ci stripe-ci removed the approved label Jan 8, 2018
@brandur-stripe
Copy link
Contributor

brandur-stripe commented Jan 8, 2018

Thanks OB. I'm glad you noticed that testing problem — it turns out that what I really wanted was PanicsWithValue instead of Panicsf. In Panicsf the message and args are just for display when the assertion fails and will have effect on the expected value. I just pushed a fix and confirmed that it indeed does fail as expected when the panic message is changed.

@brandur-stripe
Copy link
Contributor

And good catch on go-tip. I think the patch in #508 will fix it.

@brandur-stripe brandur-stripe merged commit 8ab2e41 into master Jan 8, 2018
@brandur-stripe brandur-stripe deleted the brandur-get-object-value-slices branch January 8, 2018 23:47
@brandur-stripe
Copy link
Contributor

Released as 28.9.0.

nadaismail-stripe pushed a commit that referenced this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants