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

Update clients/endpoints to use thriftrw Arg structs #92

Merged
merged 14 commits into from
May 10, 2017

Conversation

Raynos
Copy link
Contributor

@Raynos Raynos commented May 5, 2017

Currently the "structs.go" that we generate ourselves is
very limited and doesn't really support proper nested
data structures and imports.

I've changed the client and endpoints to use the
thriftrw Service_Arg_Method go structs which are generated
properly and support imports/enums/pointers etc.

This also adds "required" vs "optional" support at
the arg struct level so I had to fix a bunch of tests.

r: @uber/zanzibar-team

@coveralls
Copy link

coveralls commented May 5, 2017

Coverage Status

Coverage decreased (-0.3%) to 68.873% when pulling 994dbb6 on use-thriftrw-args into 0aa3002 on master.

Copy link
Contributor

@zhenghuiwang zhenghuiwang left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -64,8 +62,7 @@ type MethodSpec struct {
ExceptionsIndex map[string]ExceptionSpec
ValidStatusCodes []int
// Additional struct generated from the bundle of request args.
RequestBoxed bool
RequestStruct []StructSpec
RequestBoxed bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, we don't support RequestBoxed=false case, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we kind of support it. We want to get rid of it, but that's a different PR.

@@ -16,19 +16,3 @@ func getDirName() string {
_, file, _, _ := runtime.Caller(0)
return zanzibar.GetDirnameFromRuntimeCaller(file)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this template file? seems we can delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need this function getDirName() will double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok we only use this for endpoint tests, ill get rid of it for clients :)

@@ -28,7 +28,9 @@ imports:
- name: github.com/kardianos/osext
version: c2c54e542fb797ad986b31721e1baedf214ca413
- name: github.com/mailru/easyjson
version: 9d6630dc8c577b56cb9687a9cf9e8578aca7298a
version: c7e8f2b687054e448f35ff2693bd298c1a2f3a10
repo: https://github.com/Raynos/easyjson.git
Copy link
Contributor

Choose a reason for hiding this comment

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

If you pin a specific version of easyjson, then maybe always point to the master version of the customized repo, because then git repo will sync only with new stable version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to wait for bugfix to get merged, hopefully soon :)

Copy link
Contributor

@ChuntaoLu ChuntaoLu left a comment

Choose a reason for hiding this comment

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

I like it 👍

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

4 participants