-
Notifications
You must be signed in to change notification settings - Fork 214
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
Remove gun-specific code from stub to improve client adapter flexibility #267
Remove gun-specific code from stub to improve client adapter flexibility #267
Conversation
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.
This looks good! Some minor nitpicks and we're good to go
Sweet! Tks for the review. I'll work on these comments on the weekend while I finish the work for the mint adapter |
lib/grpc/client/adapters/gun.ex
Outdated
%{server_stream: true, channel: %{adapter_payload: adapter_payload}, payload: payload} = | ||
stream, | ||
opts | ||
) do |
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.
Let's split this match, leaving only the flow-control at the function head
%{server_stream: true, channel: %{adapter_payload: adapter_payload}, payload: payload} = | |
stream, | |
opts | |
) do | |
%{server_stream: true} = stream, | |
opts | |
) do |
And then match to extract the data below. Same for the other clause
I'm working on this issue: #242
And while working on that, I've found that
stub.ex
has a lot ofgun
specific code and that should live inside the adapter.This doesn't contains drastic code changes, I'm simply moving a small piece of code around to keep on working on adapter code.
My Idea here is to create an layer of abstraction, where each adapter implements it's own version of receiving data. For
gun
we have the mechanics of receive the headers first and then the payload.When working with Mint that's not the case. See
stream/2
docsTL;DR: The message we receive in the process might contain headers and parts of the payload (in the same message). So having a function that receives the headers and the body doesn't make to much sense.