feat: generator options are parsed into a standalone struct #479
Conversation
Lifts some generator options into a separate structure. Lifts option parameter parsing logic into a standalone function. Adds parsing logic for permitted transports. Also adds unit tests for parsing transport options
google-cla
bot
added
the
cla: yes
internal/gengapic/gengapic_test.go
Outdated
| if tst.expectErr { | ||
| if err == nil { |
There was a problem hiding this comment.
This is actually very confusing: for some reason, if I lift the err == nil check into the top level expression, it causes spurious test failures for the last test case. Does anyone have any idea as to why this might be?
There was a problem hiding this comment.
if tst.expectErr && (err == nil) doesn't work? Weird....
There was a problem hiding this comment.
Suggestion for debugging this: have the test print ("%v", err). I worry this may hide a simple bug, so we should try to resolve this.
There was a problem hiding this comment.
I think I figured it out: the test struct is storing the expected options by value, but ParseOptions is returning by pointer, so the comparison fails because sometimes the returned options is nil, and the expected options cannot be nil. Changing the expected options field to a pointer fixed this.
| } | ||
|
|
||
| // Default is just grpc | ||
| if opts.transports == nil { | ||
| opts.transports = []Transport{grpc} |
There was a problem hiding this comment.
Consider moving this assignment to line 102 to better group the initialization together.
There was a problem hiding this comment.
I think the current placement better communicates the intention that this is a default value if the user doesn't provide a transport= argument.
| grpcConfPath string | ||
| serviceConfigPath string | ||
| sampleOnly bool | ||
| transports []Transport |
There was a problem hiding this comment.
I believe Java assumes that either gRPC or REST will be used at a particular time. Does this array imply that callers can pass in both transport types?
There was a problem hiding this comment.
As per the design doc, the generators will support specifying which transports to generate code for via the transport option, which takes a +-separated list. The eventual default will be grpc+rest. The end developer can then specify which of these generated transports to use when communicating with the server.
| grpcConfPath string | ||
| serviceConfigPath string | ||
| sampleOnly bool | ||
| transports []Transport |
There was a problem hiding this comment.
As per the design doc, the generators will support specifying which transports to generate code for via the transport option, which takes a +-separated list. The eventual default will be grpc+rest. The end developer can then specify which of these generated transports to use when communicating with the server.
internal/gengapic/gengapic_test.go
Outdated
| if tst.expectErr { | ||
| if err == nil { |
There was a problem hiding this comment.
if tst.expectErr && (err == nil) doesn't work? Weird....
|
It's quite inexplicable and caused me at least an hour of frustration before I just gave up. |
Add more parsing tests Comment tweak Check for unexpected error Use reflect.DeepEqual instead of handrolled equality checks Add additional option parsing tests
internal/gengapic/gengapic_test.go
Outdated
| if tst.expectErr { | ||
| if err == nil { |
There was a problem hiding this comment.
Suggestion for debugging this: have the test print ("%v", err). I worry this may hide a simple bug, so we should try to resolve this.
software-dov
added
automerge
Lifts some generator options into a separate structure.
Lifts option parameter parsing logic into a standalone function.
Adds parsing logic for permitted transports.
Also adds unit tests for parsing transport options