feat: Client #5
feat: Client #5
Conversation
amplitude/client.go
Outdated
| timeline Timeline | ||
| type Amplitude interface { | ||
| Track(event BaseEvent) | ||
| Identify(identifyObj Identify, eventOptions EventOptions, eventProperties map[string]interface{}) |
There was a problem hiding this comment.
I saw in the version 1.18, we can use any to replace empty interface interface{}, see golang/go#49884.
1.18 is a major release in Q1 this year, are we considering upgrade to use it? I saw we are using 1.17: https://github.com/amplitude/Amplitude-Go/blob/main/go.mod#L3.
There was a problem hiding this comment.
Bumping up a major version might become a blocker for some customers for adopting the SDK. I wonder if we can create an alias in the meantime to call interface{} as any
There was a problem hiding this comment.
Put this to slack discussion
amplitude/client.go
Outdated
| }, | ||
| } | ||
|
|
||
| identifyEvent.loadEventOptions(eventOptions) |
There was a problem hiding this comment.
do we need a separate method for this of can we use pass it to BaseEvent{}?
There was a problem hiding this comment.
Since Go doesn't have init function for class, and child class can't inherit init function from parent class. If I use init function, I have to write three similar functions for GroupIdentifyEvent, IdentifyEvent, and RevenueEvent. But if I use loadEventOptions, I only need to write one.
Do you have any other idea to implement this without loadEventOptions?
There was a problem hiding this comment.
As discussed here, removed IdentifyEvent, GroupIdentifyEvent and RevenueEvent, and set specific event properties like eventType directly when creating them.
1. rename Amplitude to Client 2. remove Add() and Remove() return value 3. update to pointer receiver
Summary
Amplitudeinterface andNewAmplitudeas the init function foramplitudestructChecklist
Questions & Request for review
Add,Remove, andShutdownwhich updates plugins and changes the client instance itself, I use pointer receiver there. I am wondering if that is right.BaseEvent, linter says some fields are missing. As for Go standard, do I need to explicitly assign empty value to the fields ofBaseEventwhen I init it?