feat!: Add improved support for notifications and events #6
Conversation
BREAKING CHANGE: - `Event` and `NotificationsEvent` are now an interfaces - `NotificationsEvent` has moved package to paddlenotification
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
collection.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| c.results = append(c.results, &Res[T]{value: any(e).(T)}) |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
type assertion must be checked (forcetypeassert)
collection.go
Outdated
| if err := json.Unmarshal(item, &t); err != nil { | ||
| return err | ||
| } | ||
| c.results = append(c.results, &Res[T]{value: t}) |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
append only allowed to cuddle with appended value (wsl)
| if err != nil { | ||
| return err | ||
| } | ||
| c.results = append(c.results, &Res[T]{value: any(e).(T)}) //nolint:forcetypeassert // we know the type is correct. |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
append only allowed to cuddle with appended value (wsl)
| if err := json.Unmarshal(item, &t); err != nil { | ||
| return err | ||
| } | ||
| c.results = append(c.results, &Res[T]{value: t}) //nolint:forcetypeassert // we know the type is correct. |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
append only allowed to cuddle with appended value (wsl)
vifer
force-pushed
the
feature/notifications-and-events
branch
from
b54ec2c to
82487b0
Compare
collection.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
File is not gci-ed with --skip-generated -s standard -s prefix(github.com/PaddleHQ/paddle-go-sdk) -s prefix(github.com/PaddleHQ) -s default --custom-order (gci)
vifer
force-pushed
the
feature/notifications-and-events
branch
10 times, most recently
from
cbc6b9a to
0b06b2a
Compare
| // here v could be used as concrete type TransactionUpdatedEvent | ||
| fmt.Println(v.EventID) | ||
| fmt.Println(v.Data.ID) | ||
| default: |
There was a problem hiding this comment.
Thoughts about making a case for GenericEvent (I believe it should be this type of it's an event not handled by current SDK) and then having default as more of an unhandled case, log and error?
There was a problem hiding this comment.
I guess it depends of what we want to do with it, there might be cases where you only care about couple of events like we do in the example and the rest you just want to log it and nothing else.
There was a problem hiding this comment.
Having a GenericEvent (or UnknownEvent?) is less disruptive than erroring.
If a new event is subscribed to, we wouldn't want that to potentially break an integration running in production. If the integrators implementation isn't handling a default case (or GenericEvent) there's still a chance that subscribing to a new event causes an error on their side, but perhaps lower risk?
In any case we should perhaps document that integrators handle "unknown events"
vifer
force-pushed
the
feature/notifications-and-events
branch
from
0b06b2a to
de769d0
Compare
vifer
force-pushed
the
feature/notifications-and-events
branch
from
de769d0 to
ad03154
Compare
This change brings improvements in Notification and event handling. This gives us better coverage for:
/events/notificationsBREAKING CHANGE:
EventandNotificationsEventare now an interfacesNotificationsEventhas moved package to paddlenotification