-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: Context structure #59
Conversation
b5cbe57
to
60a01bb
Compare
60a01bb
to
b310b69
Compare
b310b69
to
4be06a7
Compare
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.
Would be nice with a test, but nothing blocking
@@ -83,10 +83,14 @@ func (e Confidence) PutContext(key string, value interface{}) { | |||
e.contextMap[key] = value | |||
} | |||
|
|||
func (e Confidence) Track(ctx context.Context, eventName string, message map[string]interface{}) *sync.WaitGroup { | |||
newMap := e.GetContext() | |||
func (e Confidence) Track(ctx context.Context, eventName string, data map[string]interface{}) *sync.WaitGroup { |
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.
nit: eventName should be the first argument
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.
the first context is coming from go, it's not confidence context. i think it's pretty standard to have the go context as the first parameter. you still think we should move it?
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.
Ah you are right, no probably makes sense to keep it there then
No description provided.