feat: rework response types to use interface and allow user to pass []byte or string messages to a topic #72
Conversation
…lient-sdk-go into feat/handle-byte-string-msgs
eaddingtonwhite
changed the title
| Text: request.Value, | ||
| switch request.Value.(type) { | ||
| case string: | ||
| _, err := client.unaryGrpcClient.Publish(ctx, &pb.XPublishRequest{ |
There was a problem hiding this comment.
pb "github.com/momentohq/client-sdk-go/internal/protos"
this makes this line unnecessarily hard to understand
There was a problem hiding this comment.
Yeah I agree. I need to look at how were generating the proto stubs again. I think maybe we can rework this to stop IDE from wanting us to do import alias like this. I can play with it some more and do follow up pr.
|
Sorry @eaddingtonwhite I haven't had a chance to look at this earlier. Today I reviewed it in the github UI but still haven't had time to pull it down and play with it in an IDE. So far I am on board with all of @kvcache 's suggestions. Do we have a path forward that we're all on the same page about or are there any particular things you need more 👀 on? |
| Key interface{} | ||
| Value interface{} |
There was a problem hiding this comment.
These seem not great, and like a footgun to the user. Any reason not to use type momento.Bytes interface { isBytes() } and struct RawBytes + struct StringBytes?
There was a problem hiding this comment.
Yes totally. was thinking about this as well after this pr and iteration on types. I was going to add to this PR last night but this is getting so big already I was thinking could make this change in a follow up PR. Probably ok for this PR since that functionality is not changing today from where it was. That work for you? can tag u in follow up pr also.
There was a problem hiding this comment.
A follow up for this sounds great!
| // CacheGetHit Hit Response to a cache Get api request. | ||
| type CacheGetHit struct { | ||
| Value []byte | ||
| } |
|
No I think this is gtg @cprice404 im in alignment w/ @kvcache also I think have all changes we want in for now. I want to make the one more change to CacheGet Key and Body that we were talking in follow up pr though since this is getting so large. So if I can get final 👍 from either you or @kvcache will merge this one now I think should be gtg. |
No description provided.