-
Notifications
You must be signed in to change notification settings - Fork 8
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
Receive and pass context.Context
by value in IndexConnection
methods
#20
Conversation
…a pointer in IndexConnection and its methods, update README
@@ -1,5 +1,5 @@ | |||
> **Warning** | |||
> |
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.
Prettier did a bunch of auto-formatting on some of this locally, sorry about that. 😬
@@ -4,12 +4,13 @@ import ( | |||
"context" | |||
"crypto/tls" | |||
"fmt" | |||
"log" |
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.
My gofmt settings seem to rearrange imports. Should look into turning that off maybe...
@@ -46,7 +47,7 @@ func (idx *IndexConnection) Close() error { | |||
return err | |||
} | |||
|
|||
func (idx *IndexConnection) UpsertVectors(ctx *context.Context, in []*Vector) (uint32, error) { | |||
func (idx *IndexConnection) UpsertVectors(ctx context.Context, in []*Vector) (uint32, error) { |
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 think this is fine, especially given akCtx
returns by value, and you pass context.Context
in Client
methods. At least that's my understanding.
context.Context
by value in IndexConnection
context.Context
by value in IndexConnection
methods
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.
Looks good!
Just double check on the Context
copy.
Sorry, should have read more carefully. Thanks for taking care of this.
Problem
From this issue: #19
We're receiving and passing
*context.Context
inIndexConnection
methods. This differs from ourClient
methods and is non-standard for how most libraries handleContext
.Solution
context.Context
by value rather than a pointer inIndexConnection
methods, update README example to account.Type of Change
Test Plan
CI Testing, make sure you can
just test
locally orgo build