Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Bug] Unusual use of *context.Context #19

Closed
2 tasks done
robfig opened this issue Apr 30, 2024 · 2 comments
Closed
2 tasks done

[Bug] Unusual use of *context.Context #19

robfig opened this issue Apr 30, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@robfig
Copy link

robfig commented Apr 30, 2024

Is this a new bug?

  • I believe this is a new bug
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

Updating from v0.3 to v0.4, I like the new API, much easier to use. Nice work!

However, I did notice that the API methods all accept a *context.Context instead of a context.Context. That is highly unusual. Every other usage of context.Context inside the stdlib and other API clients is context.Context, which makes me think it was unintentional. Taking address-of to satisfy the interface is no big deal, but it does seem like an API design error.

Expected Behavior

Use context.Context, instead of a pointer to that interface.

Steps To Reproduce

N/A

Relevant log output

No response

Environment

- **OS**:
- **Language version**:
- **Pinecone client version**:

Additional Context

No response
@robfig robfig added the bug Something isn't working label Apr 30, 2024
@austin-denoble austin-denoble mentioned this issue May 1, 2024
Merged
7 tasks
@austin-denoble
Copy link
Contributor

Thanks for the callout, @robfig. I think you're right, I've got a PR up to tweak the API: #20

Props to @haruska for getting the improved v0.4 client released!

austin-denoble added a commit that referenced this issue May 1, 2024
@austin-denoble
Receive and pass context.Context by value in IndexConnection meth…
32391bc 
…ods (#20)

## Problem
From this issue: #19

We're receiving and passing `*context.Context` in `IndexConnection`
methods. This differs from our `Client` methods and is non-standard for
how most libraries handle `Context`.

## Solution
- Swap to receiving and passing `context.Context` by value rather than a
pointer in `IndexConnection` methods, update README example to account.

## Type of Change
- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update
- [ ] Infrastructure change (CI configs, etc)
- [ ] Non-code change (docs, etc)
- [ ] None of the above: (explain here)

## Test Plan
CI Testing, make sure you can `just test` locally or `go build`
@austin-denoble
Copy link
Contributor

v0.5.0 has been released which should address this. Thanks again! https://github.com/pinecone-io/go-pinecone/releases/tag/v0.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@robfig @austin-denoble