Generate req/resp converters with nested structs #97
Conversation
codegen/type_converter.go
Outdated
|
|
||
| // ConvertFields will add lines to the TypeConverter for mapping | ||
| // from one go struct to another based on two thriftrw.FieldGroups | ||
| func (c *TypeConverter) ConvertFields( |
There was a problem hiding this comment.
without looking at the code, to me the method signature (name and parameters) reads like converting fields between the two, while it actually generates code for conversion. IMO, func GenConversionCode(keyPrefix string, from []*compile.FieldSpec, to []*compile.FieldSpec, helper *packageHelper) ([]string, error) is a more intuitive signature.
| // TypeConverter can generate a function body that converts two thriftrw | ||
| // FieldGroups from one to another. It's assumed that the converted code | ||
| // operates on two variables, "in" and "out" and that both are a go struct. | ||
| type TypeConverter struct { |
There was a problem hiding this comment.
Is a converter struct necessary? feels like the convertFields method can be a standalone function.
There was a problem hiding this comment.
a standalone function passes too many args around so it put it on an object.
| line = "}" | ||
| c.Lines = append(c.Lines, line) | ||
|
|
||
| default: |
There was a problem hiding this comment.
Maybe default should return error?
There was a problem hiding this comment.
It should once we support other types, implementation is partial atm.
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Unit tests on this would be nice.
There was a problem hiding this comment.
I wrote ALL the unit tests.
| end=`date +%s` | ||
| runtime=$((end-start)) | ||
| echo "Generated easy_json files for endpoints +$runtime" | ||
| # TODO : Add new post steps :) |
There was a problem hiding this comment.
mind expanding the comment to list what kinds of post steps?
There was a problem hiding this comment.
I dont think we have any post steps at the moment. I can delete this file.
codegen/method.go
Outdated
| RequestTypeMap map[string]string | ||
|
|
||
| // Statements for converting request types | ||
| ConvertRequestLines []string |
There was a problem hiding this comment.
maybe change "Lines" to "GolangStatements" or something that makes it clear that these are lines of go code and the go code here is being build ahead of time by the convert function and not by the templates.
There was a problem hiding this comment.
also, we need some tests on the type converter.
Raynos
force-pushed
the
convert-nested-bodies
branch
from
8c17060
to
8e2a251
Compare
|
I addressed all comments and wrote tests. cc @stevededalus @ChuntaoLu |
This PR improves our existing request / response converter
functions to copy over nested structs.
I've significantly expanded and refactor the approach for
generating these and moved logic into a seperate file.
There is more work still to do be done that will come in a
seperate PR, for example lists, maps, sets, enums, etc.
r: @uber/zanzibar-team