Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
V1: Initial server implementation #593
V1: Initial server implementation #593
Changes from all commits
34e698a
3a75901
af72cf4
62f52d5
829dcd6
3576e35
23a6333
f63a434
6078d78
cce7962
ee5dc64
643f760
da0474b
08ccef3
1f6dbd8
0cb037d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 could probably be pulled out into its own function also. Lmk if you think it is worth it.
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.
In my mind, this isn't really "parsing". Maybe just call it
createResponse
?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 thought of using
parse
since it evaluates theoneof
and can return a response or a Connect error based on that response. In light of that, do you think thatcreateResponse
makes sense?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 guess I think or "parse" differently than that. It's fine either way -- not a strongly held opinion :)
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.
You are in the the case where you know this is the right type. And since you already have
rt :=
in the switch condition, the compiler already knows thatrt
is a*v1alpha1.UnaryResponseDefinition_ResponseData
. So you shouldn't need this.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 think this is needed since the case is defined as
case *v1alpha1.UnaryResponseDefinition_ResponseData, nil:
. Otherwise, I get a compiler error:rt.ResponseData undefined