-
Notifications
You must be signed in to change notification settings - Fork 112
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.
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
Use generics for subscriptions responses #356
base: main
Are you sure you want to change the base?
Conversation
90626ca
to
dd35e90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I know this is still a draft, just figured I'd take a peek -- feel free to ignore!)
This is a neat idea!
I worry that it changes the interface a bit too much though. In the tests it's fine but in real code you often pass around the client or attach it to some struct used for many queries (DI-style).
Of course the most straightforward way would be if we could scope generics to methods, but Go doesn't allow that. I wonder if we could do something equivalent-ish by having two structs:
- client (as-is) which users pass around
- per-query-client (generic) which we construct inline, use for the query, and then throw away
That still changes theClient
interface probably, which is not ideal but more okay (and for subscriptions it's totally fine since it's brand new). Although maybe then it doesn't help that much, if the interface between 1 and 2 needs to be too complex. Anyway, I haven't thought this through, just sharing an idea!
bf9a285
to
9dd475e
Compare
@benjaminjkraft Thanks a lot for your feedback! I have thought about this a lot, and I think you are absolutely right. Introducing generics on the client layer like I did, would worsen the user experience. So it's not worth doing. I will reduce the scope of this PR and still see if I find any useful refactor using generics 🤗 |
bc7cce7
to
4f08a07
Compare
4f08a07
to
74c228f
Compare
Hi @benjaminjkraft, I landed on this quite minimal refactor in the end. Let me know if you think it's useful or not. I could also be fine with throwing this away. My real goal was to avoid the type casting of |
No description provided.