This repository has been archived by the owner on Sep 26, 2023. It is now read-only.
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.
feat: REST Gapic (REGAPIC) Support #1177
feat: REST Gapic (REGAPIC) Support #1177
Changes from 8 commits
85bb35d
d6c2829
55b9f21
3c4c19d
aee533a
e02e368
7c08c24
d264bc6
3a982ff
a71e5d8
07b20ba
7f72f2b
0c0328b
5f257e6
71474be
b231f0f
d3b13f9
79c41a1
0dc2d89
169fcf2
2619f63
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.
nit: no period per Oracle guidelines
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.
Corrected. It seems like we don't have consistency regarding this in our gax library (some docs use dots some don't)
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.
no period
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.
Corrected
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.
triple nested generics strongly suggests that there's a more detailed object type should be created for this. I.e. avoid FieldsExtractor of Map of List.
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 agree that three nested generics is too much, but unfortunately I'm forced to do it here. This field is used solely to implement the
Map<String, List<String>> getQueryParamNames(MessageFormatT apiMessage);
method of this class, which, in its turn, is declared in the interface implemented by this class (HttpRequestFormatter<MessageFormatT>
). I.e. this triple generic thing was predetermined by the existing architecture which I really don't want to mess with unless it is absolutely necessary.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.
Could you please add a brief comment to this effect, so that future well-meaning readers won't try to "fix" 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 am increasingly less and less happy with GAX and GSON. Their mistakes are cascading down the dependency tree. :-(
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 don't understand why a method called getQueryParamNames is returning a map? Names would seem to be a list.
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.
It is implementing an existing interface. As I understand it, key is a name of a parameter, List is its values (to support array query parameters):
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'm curious, and maybe I'll find the answer below: when does a client need to serialize a response message into a (JSON) string? Isn't it always paring JSON into messages? Or are we making this available purely for use by servers?
(it might be nice to note the answer in a comment in the interface)
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 was wondering the same thing. I don't have a good answer for you =).
Note, this class implements an interface (
HttpResponseParser
), so I'm basically forced here to implement it. At the same time I can't find any prod usages of this method. It is used only once in a test, I'm not sure why. In other words, the safest and simplest thing was just to implement this method properly, especially taking into account that it takes only one line of code.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.
OK. Maybe a brief comment inside the function to the effect that we're not using this in prod but this is needed by the interface?