Closed elee1766 closed 3 months ago
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?
@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.
@kishorenc see https://github.com/typesense/typesense-go/pull/158
Looks good, PR ready to merge?
I mean, the PR with the change to Uber's mock
library.
I mean, the PR with the change to Uber's
mock
library.
yep, that one is good to merge.
there's probably an argument to make the default type any
instead of map[string]any
the reason is because
the reasons against are the same as the reasons for
personally i think i still prefer map[string]any - typesense documents are objects and showing that seems appropriate
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.
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.
@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.
ah ok. will do when i find some time.
@kishorenc sorry for delay, i wrote this test.
does this make sense?
@elee1766
Thanks, that looks good. Is this branch good to merge?
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 https://github.com/typesense/typesense-go/pull/156#issuecomment-2130631960, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARH66M2LKSEI3LPVOCAPMLZD7PHPAVCNFSM6AAAAABFKBKE7KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZQGYZTCOJWGA . You are receiving this because you were mentioned.Message ID: @.***>
I've published v2.0.0
that contains this change and also latest API updates. Thank you for your contribution!
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