Skip to content
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

Set full type for request and respone #105

Merged
merged 1 commit into from
May 24, 2017
Merged

Set full type for request and respone #105

merged 1 commit into from
May 24, 2017

Conversation

ChuntaoLu
Copy link
Contributor

Currently we assume all request and response types are struct pointer type, and prefix * in our templates. This assumption is not always valid.
This PR sets the full request and response type without assuming it is a struct pointer type.

@coveralls
Copy link

coveralls commented May 22, 2017

Coverage Status

Coverage increased (+0.1%) to 73.917% when pulling 0089061 on lu.type into ab97b38 on master.

Copy link
Contributor

@Raynos Raynos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The req / res converters are broken for primitive request / responses.

We need to at least fix the response path, we plan to deprecate the "ref.boxed" stuff for the request path ( aka request will always be the Arg struct ).

@@ -198,12 +198,15 @@ func (ms *MethodSpec) setRequestType(curThriftFile string, funcSpec *compile.Fun
ms.RequestType, err = packageHelper.TypeFullName(
curThriftFile, funcSpec.ArgsSpec[0].Type,
)
if err == nil && isStructType(funcSpec.ArgsSpec[0].Type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add TODO here to check typedef semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typedef should be covered since the type assertion is against the RootTypeSpec of given spec. https://github.com/uber/zanzibar/blob/master/codegen/method.go#L217

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#coolstory

@@ -225,6 +228,9 @@ func (ms *MethodSpec) setResponseType(curThriftFile string, respSpec *compile.Re
return nil
}
typeName, err := packageHelper.TypeFullName(curThriftFile, respSpec.ReturnType)
if isStructType(respSpec.ReturnType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a TODO here to check typedef semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

func convertTo{{title .Name}}ClientRequest(in *{{.RequestType}}) *{{$clientReqType}} {
out := &{{$clientReqType}}{}
func convertTo{{title .Name}}ClientRequest(in {{.RequestType}}) {{$clientReqType}} {
out := &{{unref $clientReqType}}{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&string{} will probably break here ?

We need some kind of "is primitive" check and not generate converters for non-struct types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good point.

Copy link
Contributor

@Raynos Raynos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idealy we should have validation that the response type for endpoints is "struct only" and the response type for clients can be anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants