Add docstrings for client.go #27

aulorbe · 2024-06-20T21:07:53Z

Problem

We do not have docstrings in the public types, functions, and methods in client.go.

Asana ticket: https://app.asana.com/0/1203260648987893/1207208972408364/f

Solution

Add docstrings!

Type of Change

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 passes.

pinecone/client.go

austin-denoble

Great work getting all of these doc comments written, there was a lot of ground to cover here.

Overall I think the formatting is solid, nice job. I've been reviewing and learning more as I go from https://tip.golang.org/doc/comment.

Most of my feedback is around the NewClient and NewClientBase functions and the associated NewClientParams and NewClientBaseParams structs, since that's a lot of how folks will configure and work with `Client.

There's a lot of nit formatting and indentation feedback in here too, I was seeing things across the board so tried to leave limited comments for now. I can take another pass when you're ready, but this is looking solid so far. 🎉

austin-denoble · 2024-06-24T20:35:46Z

pinecone/client.go

+// The Client is designed to be long-lived and reused across multiple operations.
+//
+// Fields:
+//   - headers: A map of additional HTTP headers to include in the API request.


nit: We may want to be explicit here:

Suggested change

// - headers: A map of additional HTTP headers to include in the API request.

// - headers: An optional map of additional HTTP headers to include in each API request to the control plane. Provided through NewClientParams.Headers or NewClientBaseParams.Headers..

godoc should automatically link to NewClientParams.Headers. I've also realized this represents what ends up in this field currently, but maybe we'd want to change it at some point to reflect what is actually sent through on requests via clientOptions as opposed to only what's passed:

go-pinecone/pinecone/client.go

Line 79 in 3278f6d

c := Client{restClient: client, sourceTag: in.SourceTag, headers: in.Headers}

austin-denoble · 2024-06-24T20:43:56Z

pinecone/client.go

+// It encapsulates the necessary authentication, request creation, and response handling for the API's operations.
+//
+// The Client is designed to be long-lived and reused across multiple operations.


I don't know if this needs to be its own line, could it just be part of the first couple sentences / paragraph?

Also, do you think it makes sense to add another paragraph here around using NewClient or NewClientBase to instantiate the Client struct rather than directly? I just want to be clear and explicit about how the structs and functions in the package are used, and we do a lot of the lifting for them.

austin-denoble · 2024-06-24T20:45:33Z

pinecone/client.go

+//   - headers: A map of additional HTTP headers to include in the API request.
+//   - restClient: The underlying REST client used to communicate with the Pinecone control plane API.
+//     This field is internal and managed by Client.
+//   - sourceTag: An optional string used to help Pinecone attribute API activity to our partners.


nit: Drop the "our" here? Just sounds a bit formal. Also we can link to the relevant fields in the NewClient* structs.

Suggested change

// - sourceTag: An optional string used to help Pinecone attribute API activity to our partners.

// - sourceTag: An optional string used to help Pinecone attribute API activity to partners. Provided through NewClientParams.SourceTag or NewClientBaseParams.SourceTag.

austin-denoble · 2024-06-24T21:03:36Z

pinecone/client.go

+// To use Client, first build the parameters of your request using NewClientParams,
+// providing your API key. Then pass those parameters into the NewClient function to create a new Client.
+// Once instantiated, you can call Client's methods to perform actions such as creating, deleting,
+// and describing indexes and collections.


I'd maybe word things like this here:

Suggested change

// To use Client, first build the parameters of your request using NewClientParams,

// providing your API key. Then pass those parameters into the NewClient function to create a new Client.

// Once instantiated, you can call Client's methods to perform actions such as creating, deleting,

// and describing indexes and collections.

// To create a Client instance, use the NewClient function with an instance of NewClientParams to pass

// configuration options such as providing your API key. Once instantiated, you can call Client's methods

// to perform actions such as creating, deleting, and describing indexes and collections.

While we use the configuration parameters in requests, they're not really instantiating a Client on a per-request basis.

austin-denoble · 2024-06-24T21:04:57Z

pinecone/client.go

+//	if err != nil {
+//	  fmt.Println("Error:", err)
+//	 }
+//
+//	for _, idx := range idxs {
+//	  fmt.Println(idx)
+//	 }


I don't think the code should be indented here, should align with the idxs, err line.

austin-denoble · 2024-06-24T22:10:49Z

pinecone/client.go

+//	if err != nil {
+//	  fmt.Println("Error:", err)
+//	 }


indentation

austin-denoble · 2024-06-24T22:13:34Z

pinecone/client.go

+//   }
+//
+//  pc, err := pinecone.NewClient(clientParams)
+//  if err != nil {
+//    log.Fatalf("Failed to create Client: %v", err)
+//   }


nit: Is it just me or are these closing brackets indented one space? I keep thinking I'm seeing indentation problems but it's hard to tell.

austin-denoble · 2024-06-24T22:19:15Z

pinecone/client.go

 	return c.IndexWithAdditionalMetadata(host, "", nil)
 }

+// IndexWithNamespace creates an IndexConnection to the specified host within the specified namespace.


Should we add "If namespace is not explicitly provided the default of "" will be used."?

austin-denoble · 2024-06-24T22:23:37Z

pinecone/client.go

+// IndexWithAdditionalMetadata creates an IndexConnection to the specified host within the specified namespace,
+// with the addition of custom metadata fields.


additionalMetadata can be thought of analogous to Headers in the NewClient function. They're metadata that's applied to each RPC request to the server: https://grpc.io/docs/guides/metadata/. I think we could be explicit about that here, and maybe even link to some of the supporting gRPC documentation.

austin-denoble · 2024-06-24T22:24:24Z

pinecone/client.go

+//   }
+//
+//  pc, err := pinecone.NewClient(clientParams)
+//  if err != nil {
+//    log.Fatalf("Failed to create Client: %v", err)
+//   }
+//
+//  idxs, err := pc.ListIndexes(ctx)
+//	if err != nil {
+//	  fmt.Println("Error:", err)
+//	 }
+//
+//  idx, err := pc.IndexWithAdditionalMetadata(
+//                idxs[0].Host,
+//                <"sample-namespace">,
+//                map[string]string{"custom-request-metadata": "custom-metadata-values"}
+//              ) // You can now use idx to interact with your index.


nit: More indentation woes 😬

Yeah my linter doesn't do indentation for comments. Sorry about that!

jseldess · 2024-06-25T16:01:16Z

pinecone/client.go

+//   - Shards: The number of shards to use for the index (defaults to 1).
+//   - Replicas: The number of replicas to use for the index (defaults to 1).
+//   - SourceCollection: The Collection from which to create the index.
+//   - MetadataConfig: The metadata configuration for the index.


Would be nice to include metadata config in the example below.

pinecone/client.go

This reverts commit bfc0d2f.

austin-denoble

🎉 Nice work! This looks great, I think we're good to go ahead and merge whenever you're ready. Thanks again for taking all these doc comments on and being so good about iterating and feedback.

Some questions, nits, etc here. I don't think there's anything necessarily blocking though.

austin-denoble · 2024-07-01T17:45:48Z

pinecone/client.go

+// Fields:
+//   - headers: An optional map of additional HTTP headers to include in each API request to the control plane,
+//   provided through NewClientParams.Headers or NewClientBaseParams.Headers.
+//   - restClient: The underlying REST client used to communicate with the Pinecone control plane API.


nit: Could mention the NewClientParams.RestClient / NewClientBaseParams.RestClient like the other values here. Also that it specifically accepts an *http.Client, and that this is optional and we create the client for you if it isn't provided.

austin-denoble · 2024-07-01T18:00:27Z

pinecone/client.go

+//	  log.Println("IndexConnection created successfully!")
+//  }
+//
+// [pinecone.io/docs]: https://docs.pinecone.io/reference/api/control-plane/list_indexes


nit: Should the rendered string for this link be:

Suggested change

// [pinecone.io/docs]: https://docs.pinecone.io/reference/api/control-plane/list_indexes

// [docs.pinecone.io/reference/api]: https://docs.pinecone.io/reference/api/control-plane/list_indexes

idk, maybe you were trying to keep it shorter, but it feels like it should align with the underlying URL.

Yeah I was just trying to keep it short. Maybe I'll just hyperlink plain text instead of the shortened URL.

austin-denoble · 2024-07-01T18:04:53Z

pinecone/client.go

+// NewClientParams holds the parameters for creating a new Client instance while authenticating via an API key.
+//
+// Fields:
+//   - ApiKey: The API key used to authenticate with the Pinecone control plane API.


I would mention that ApiKey is the only required field here, and that it must be provided unless you're providing it through the environment variable PINECONE_API_KEY, in which case it's not required here. If the API key is not provided when creating a new Client instance, you'll receive an error.

Something like that.

austin-denoble · 2024-07-01T18:14:04Z

pinecone/client.go

+// This function sets up the control plane client with the necessary configuration for authentication and communication.
+//
+// Parameters:
+//   - in: A NewClientParams object. See NewClientParams for more information.


Just curious - does godoc automatically find the references for these if we're referring to functions or types within the same package, or do we have to link to things manually?

In my IDE rendering, they're auto-found

austin-denoble · 2024-07-01T18:15:22Z

pinecone/client.go

@@ -59,6 +151,34 @@ func NewClient(in NewClientParams) (*Client, error) {
 	return NewClientBase(NewClientBaseParams{Headers: clientHeaders, Host: in.Host, RestClient: in.RestClient, SourceTag: in.SourceTag})
 }

+// NewClientBase creates and initializes a new instance of Client with custom authentication headers.


nit: Might be nice to add something like "ApiKey is required when using NewClient, but is not required when using NewClientBase"? That wording feels bad, but just something to stress the "Base" impl requires you to build your own auth a bit more directly.

austin-denoble · 2024-07-01T19:13:05Z

pinecone/client.go

+//   - Shards: The number of shards to use for the index (defaults to 1).
+//   - Replicas: The number of [replicas] to use for the index (defaults to 1).


Along with my previous comments, I'd take a look at how we represent some of these fields in the OAS documentation and lift that, just for specificity:

replicas: https://github.com/pinecone-io/apis/blob/fbd9d8d48c95b12c6b28aea4e496b6f28666d2d1/src/release/control/resources/indexes/models/PodSpecReplicas.yaml#L3

shards: https://github.com/pinecone-io/apis/blob/fbd9d8d48c95b12c6b28aea4e496b6f28666d2d1/src/release/control/resources/indexes/models/PodSpecShards.yaml#L3

etc

austin-denoble · 2024-07-01T19:20:40Z

pinecone/client.go

+//   - Name: The name of the Serverless index.
+//   - Dimension: The dimension of the index (must match dimensionality of upserted vectors).
+//   - Metric: The metric used to measure the [similarity] between vectors.
+//   - Cloud: The [cloud provider] in which the index will be created.


Same feedback regarding the other index fields in the pods-based approach, maybe add a bit more specificity. You can pull directly from the OAS spec: https://github.com/pinecone-io/apis/blob/fbd9d8d48c95b12c6b28aea4e496b6f28666d2d1/src/release/control/resources/indexes/models/ServerlessSpec.yaml#L14

austin-denoble · 2024-07-01T19:21:34Z

pinecone/client.go

+//    Name:    "my-serverless-index",
+//    Dimension: 3,
+//    Metric:  pinecone.Cosine,
+//    Cloud:   pinecone.Aws,
+//    Region:  "us-east-1",
+//    },


nit: I think these fields need to be inset just a bit.

austin-denoble · 2024-07-01T19:21:51Z

pinecone/client.go

+//    Name:        "my-pod-index",
+//    Dimension:   3,
+//    Metric:      pinecone.Cosine,
+//    Environment: "us-west1-gcp",
+//    PodType:     "s1",
+//    MetadataConfig: podIndexMetadata,
+//    }


nit: Inset fields a bit.

austin-denoble · 2024-07-01T19:25:04Z

pinecone/client.go

@@ -273,6 +790,47 @@ func (c *Client) DeleteIndex(ctx context.Context, idxName string) error {
 	return nil
 }

+// ListCollections retrieves a list of all collections in a Pinecone [project]. See Collection for more information.


Does "See Collection" here needs [] around "Collection"?

aulorbe commented Jun 21, 2024

View reviewed changes

pinecone/client.go Outdated Show resolved Hide resolved

aulorbe changed the title ~~Sample docstring for Client struct~~ Add docstrings for client.go Jun 24, 2024

aulorbe commented Jun 24, 2024

View reviewed changes

pinecone/client.go Outdated Show resolved Hide resolved

aulorbe commented Jun 24, 2024

View reviewed changes

pinecone/client.go Show resolved Hide resolved

aulorbe requested review from austin-denoble June 24, 2024 18:01

aulorbe marked this pull request as ready for review June 24, 2024 18:01

austin-denoble reviewed Jun 24, 2024

View reviewed changes

jseldess reviewed Jun 25, 2024

View reviewed changes

junefish reviewed Jun 25, 2024

View reviewed changes

pinecone/client.go Outdated Show resolved Hide resolved

aulorbe force-pushed the Audrey/client-documentation branch from 763fe23 to 669951b Compare June 27, 2024 18:51

aulorbe added 14 commits June 28, 2024 15:12

Sample docstring for Client struct

ee42b2c

Save work

37220c0

Save work

454fc3d

All client docstrings, rough draft

d5994f9

Remove bug found in order to pull out into separate pr

a5cf5a9

Return ensureURLScheme back to original name

ce738b8

Finalize docstrings for review

8acfc6d

Save work

7976371

Finalizing + addressing Austin comments

a5c24e6

save work

6dd9060

Save work

5acb5a8

Finalizing friday

60dd879

More changes

ef575e9

Revert "More changes"

f5c63d6

This reverts commit bfc0d2f.

aulorbe force-pushed the Audrey/client-documentation branch from 8382f11 to f5c63d6 Compare June 28, 2024 22:12

aulorbe added 3 commits June 28, 2024 15:51

Finalize except Jesse comments

7a6014f

Add MetadataConfig obj to PodRequest obj

eaee819

Whoops; remove debug stmts

d3faee8

austin-denoble approved these changes Jul 1, 2024

View reviewed changes

aulorbe added 3 commits July 9, 2024 10:42

Update definitions to match OAS

d1087a3

Remove Serverless regions part b/c it will get stale quickly

6e48025

Capitalize Index and Collection so that they link

fe79d6d

aulorbe merged commit 479d6c7 into main Jul 9, 2024
3 checks passed

aulorbe deleted the Audrey/client-documentation branch July 9, 2024 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docstrings for client.go #27

Add docstrings for client.go #27

aulorbe commented Jun 20, 2024 •

edited

Loading

austin-denoble left a comment

austin-denoble Jun 24, 2024

austin-denoble Jun 24, 2024

austin-denoble Jun 24, 2024

austin-denoble Jun 24, 2024

austin-denoble Jun 24, 2024

austin-denoble Jun 24, 2024

austin-denoble Jun 24, 2024

austin-denoble Jun 24, 2024

austin-denoble Jun 24, 2024

austin-denoble Jun 24, 2024

aulorbe Jun 24, 2024

jseldess Jun 25, 2024

aulorbe Jun 25, 2024

aulorbe Jun 28, 2024

austin-denoble left a comment

austin-denoble Jul 1, 2024

austin-denoble Jul 1, 2024

aulorbe Jul 1, 2024

austin-denoble Jul 1, 2024

austin-denoble Jul 1, 2024

aulorbe Jul 9, 2024 •

edited

Loading

austin-denoble Jul 1, 2024

austin-denoble Jul 1, 2024

austin-denoble Jul 1, 2024

austin-denoble Jul 1, 2024

austin-denoble Jul 1, 2024

austin-denoble Jul 1, 2024

	// - headers: A map of additional HTTP headers to include in the API request.
	// - headers: An optional map of additional HTTP headers to include in each API request to the control plane. Provided through NewClientParams.Headers or NewClientBaseParams.Headers..

	// - sourceTag: An optional string used to help Pinecone attribute API activity to our partners.
	// - sourceTag: An optional string used to help Pinecone attribute API activity to partners. Provided through NewClientParams.SourceTag or NewClientBaseParams.SourceTag.

		// IndexWithAdditionalMetadata creates an IndexConnection to the specified host within the specified namespace,
		// with the addition of custom metadata fields.

	// [pinecone.io/docs]: https://docs.pinecone.io/reference/api/control-plane/list_indexes
	// [docs.pinecone.io/reference/api]: https://docs.pinecone.io/reference/api/control-plane/list_indexes

		// - Shards: The number of shards to use for the index (defaults to 1).
		// - Replicas: The number of [replicas] to use for the index (defaults to 1).

Add docstrings for client.go #27

Add docstrings for client.go #27

Conversation

aulorbe commented Jun 20, 2024 • edited Loading

Problem

Solution

Type of Change

Test Plan

austin-denoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe commented Jun 20, 2024 •

edited

Loading

aulorbe Jul 9, 2024 •

edited

Loading