-
Notifications
You must be signed in to change notification settings - Fork 7
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this to slack discussion
amplitude/client.go
Outdated
}, | ||
} | ||
|
||
identifyEvent.loadEventOptions(eventOptions) |
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.
do we need a separate method for this of can we use pass it to BaseEvent{}
?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Amplitude
interface andNewAmplitude
as the init function foramplitude
structChecklist
Questions & Request for review
Add
,Remove
, andShutdown
which 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 ofBaseEvent
when I init it?