Start on generics

[elee1766]

Change Summary

attempt to add generics support to the package.

the goal here is to partially addresses #144

since functionality can be preserved with the type parameter map[string]any, not everything needs to be done at once. additionally, existing behavior can be preserved (although people will have to change their types in code)

some things will be difficult to generify because a large amount of the package relies on openapi code generated types, for which adding generics support might be a much larger undertaking

PR Checklist

• I have read and signed the Contributor License Agreement.

[elee1766]

hey @kishorenc this is just a start but i was wondering what you thought about this (and perhaps changing more things to be like this)

i also noticed that you guys are using the deprecated mocking package - is switching to uber's mock library something you're interested in?

[kishorenc]

@elee1766

Thank you for initiating this. I'm broadly in support of adding generics if it improves the usability of the client. I will review this PR in more detail.

i also noticed that you guys are using the deprecated mocking package - is switching to uber's mock library something you're interested in?

Yes, that will be great.

[elee1766]

@kishorenc see #158

[kishorenc]

Looks good, PR ready to merge?

[kishorenc]

I mean, the PR with the change to Uber's mock library.

[elee1766]

I mean, the PR with the change to Uber's mock library.

yep, that one is good to merge.

[elee1766]

there's probably an argument to make the default type any instead of map[string]any

the reason is because

1. json.Unmarshaler will unmarshal to map[string]any by default anyways.
2. even though type sense doesnt support documents that are not objects, perhaps one day it might be able to.

the reasons against are the same as the reasons for

1. json.unmarshaler will unmarhsal to map[string]any by default anyways, so we may as well save the users the cast?
2. typesense documents are objects, so perhaps its good to force that anyways.

personally i think i still prefer map[string]any - typesense documents are objects and showing that seems appropriate

[kishorenc]

I think using map[string]any conveys the correct semantics now. In future, if that changes, then we can do a migration to any. I don't see Typesense documents not being objects.

The PR looks fine, but can you please show what type of change will be required for someone to use the updated interface? Would you be able to show a small before/after snippet for a sample collection with one to two fields. Also, how we will handle non-200 statuses as well.

[elee1766]

re: non-200 status

the functionality is preserved, so there should be no difference for users

As for migration to the new interface

before:
var x typesense.CollectionInterface
var z typesense.DocumentInterface

after:
var x typesense.CollectionInterface[map[string]any]
var z typesense.DocumentInterface[map[string]any]

before:
type S struct {
  x typesense.CollectionInterface
  z typesense.DocumentInterface
}

after:
type S struct {
  x typesense.CollectionInterface[map[string]any]
  z typesense.DocumentInterface[map[string]any]
}

all other behavior should be preserved.

[kishorenc]

@elee1766

Sorry, let me clarify. Can you give me an example usage where the user creates a custom struct and then passes it to one of the typed functions (say, document.retrieve) and get the response as a typed struct? Atleast some of the existing tests should test the typed interface this way.

I apologize -- I've not kept up with the Go generics features.

[elee1766]

ah ok. will do when i find some time.

[elee1766]

@kishorenc sorry for delay, i wrote this test.

typesense-go/typesense/test/document_test.go

Lines 14 to 22 in 31c89a0

	func TestDocumentRetrieveGeneric(t *testing.T) {
	collectionName := createNewCollection(t, "companies")
	expectedResult := newStructResponse("123")
	createDocument(t, collectionName, newDocument("123"))
	result, err := typesense.GenericCollection[*testDocument](typesenseClient, collectionName).Document("123").Retrieve(context.Background())
	require.NoError(t, err)
	require.Equal(t, expectedResult, result)
	}

does this make sense?

[kishorenc]

@elee1766

Thanks, that looks good. Is this branch good to merge?

[elee1766]

it should be, we have been using it fine. On May 24, 2024, tetra12 ***@***.***> wrote: @elee1766 <https://github.com/elee1766> Thanks, that looks good. Is this branch good to merge? — Reply to this email directly, view it on GitHub <#156 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARH66M2LKSEI3LPVOCAPMLZD7PHPAVCNFSM6AAAAABFKBKE7KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZQGYZTCOJWGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[kishorenc]

I've published v2.0.0 that contains this change and also latest API updates. Thank you for your contribution!