search

amplitude / analytics-go

Go Amplitude Analytics SDK
MIT License
12 stars 7 forks source link

fix: various fixes #36

Closed falconandy closed 2 years ago

falconandy commented 2 years ago

Summary

Various fixes, including support for:

Available as version v0.0.3

Checklist

justin-fiedler commented 2 years ago

Thanks @falconandy this is a big improvement.

Can we improve Event creation to better match the other SDKs (i.e. can we set EventOptions directly on the Event)? The current implementation seems clunky. https://github.com/amplitude/analytics-go/blob/sdk-fixes/examples/track_example/track_example.go#L18-L21

Proposed

analytics.Track(amplitude.Event{
  UserID: "user-id"
  EventType:    "Button Clicked",
})

analytics.Track(amplitude.Event{
  UserID: "user-id"
  EventType:    "Button Clicked",
}, amplitude.EventOptions({ DeviceId: "device-id"})

I understand there are no optional args in Go so not sure the best solution to support amplitude.Track(event: Event, options?: EventOptions).

Having to do amplitude.Track(event, nil) as the common case isn't great. I don't want to have to make a new method but maybe that is the best choice? E.g. amplitude.Track(event) and amplitude.TrackWithOptions(event, options)

Since TrackWithOptions would likely only be used by Ampli, maybe we only support amplitude.Track(event) and then have Ampli consolidate Event/EventOptions before calling core.

WDYT?

falconandy commented 2 years ago

@justin-fiedler In Ampli codegen Variadic Functions are used to emulate optional parameter:

func (a *Ampli) Track(userID string, event Event, eventOptions ...EventOptions) {
    var options EventOptions
    if len(eventOptions) > 0 {
        options = eventOptions[0]
    }
    ...

Usage:

ampli.Track(userId, event)
ampli.Track(userId, event, options)
justin-fiedler commented 2 years ago

Thanks @falconandy I like that. Can you please just fix the minor comments here. I made a new ticket to modify the Track usage - https://amplitude.atlassian.net/browse/AMP-60351