[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user johnboiles commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r160477076
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -1953,177 +1960,75 @@ void
t_go_generator::generate_service_client(t_service* tservice) {
f_types_ << indent() << "func (p *" << serviceName << "Client) "
<< function_signature_if(*f_iter, "", true) << " {" << endl;
indent_up();
-/*
-f_types_ <<
- indent() << "p.SeqId += 1" << endl;
-if (!(*f_iter)->is_oneway()) {
- f_types_ <<
-indent() << "d := defer.Deferred()" << endl <<
-indent() << "p.Reqs[p.SeqId] = d" << endl;
-}
-*/
-f_types_ << indent() << "if err = p.send" << funname << "(";
-bool first = true;
-
-for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) {
- if (first) {
-first = false;
- } else {
-f_types_ << ", ";
- }
- f_types_ << variable_name_to_go_name((*fld_iter)->get_name());
-}
-
-f_types_ << "); err != nil { return }" << endl;
-
-if (!(*f_iter)->is_oneway()) {
- f_types_ << indent() << "return p.recv" << funname << "()" << endl;
-} else {
- f_types_ << indent() << "return" << endl;
-}
-
-indent_down();
-f_types_ << indent() << "}" << endl << endl;
-f_types_ << indent() << "func (p *" << serviceName << "Client) send"
- << function_signature(*f_iter) << "(err error) {" << endl;
-indent_up();
-std::string argsname = publicize((*f_iter)->get_name() + "_args",
true);
-// Serialize the request header
-f_types_ << indent() << "oprot := p.OutputProtocol" << endl;
-f_types_ << indent() << "if oprot == nil {" << endl;
-f_types_ << indent() << " oprot =
p.ProtocolFactory.GetProtocol(p.Transport)" << endl;
-f_types_ << indent() << " p.OutputProtocol = oprot" << endl;
-f_types_ << indent() << "}" << endl;
-f_types_ << indent() << "p.SeqId++" << endl;
-f_types_ << indent() << "if err = oprot.WriteMessageBegin(\"" <<
(*f_iter)->get_name()
- << "\", " << ((*f_iter)->is_oneway() ? "thrift.ONEWAY" :
"thrift.CALL")
- << ", p.SeqId); err != nil {" << endl;
-indent_up();
-f_types_ << indent() << " return" << endl;
-indent_down();
-f_types_ << indent() << "}" << endl;
-f_types_ << indent() << "args := " << argsname << "{" << endl;
+std::string method = (*f_iter)->get_name();
+std::string argsType = publicize(method + "_args", true);
+std::string argsName = tmp("_args");
+f_types_ << indent() << "var " << argsName << " " << argsType << endl;
for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) {
- f_types_ << indent() << publicize((*fld_iter)->get_name()) << " : "
- << variable_name_to_go_name((*fld_iter)->get_name()) <<
"," << endl;
+ f_types_ << indent() << argsName << "." <<
publicize((*fld_iter)->get_name())
+ << " = " <<
variable_name_to_go_name((*fld_iter)->get_name()) << endl;
}
-f_types_ << indent() << "}" << endl;
-
-// Write to the stream
-f_types_ << indent() << "if err = args." << write_method_name_ <<
"(oprot); err != nil {" << endl;
-indent_up();
-f_types_ << indent() << " return" << endl;
-indent_down();
-f_types_ << indent() << "}" << endl;
-f_types_ << indent() << "if err = oprot.WriteMessageEnd(); err != nil
{" << endl;
-indent_up();
-f_types_ << indent() << " return" << endl;
-indent_down();
-f_types_ << indent() << "}" << endl;
-f_types_ << indent() << "return oprot.Flush()" << endl;
-indent_down();
-f_types_ << indent() << "}" << endl << endl;
if (!(*f_iter)->is_oneway()) {
- std::string resultname = publicize((*f_iter)->get_name() +
"_result", true);
- // Open function
- f_types_ << endl << indent() << "func (p *" << serviceName <<
"Client) recv"
- << publicize((*f_iter)->get_name()) << "() (";
+ std::string resultName = tmp("_result");
+ std::string resultType = publicize(method + "_result", true);
+ f_types_ << indent() << "var " << resultName << " " << resultType <<
endl;
+ f_types_ << indent() << "if err = p.c.Call(ctx, \""
--- End diff --
When using one of the now-deprecated initializers, this line will cause a
panic, since `p.c` is nil. The deprecated initializers create the `TClient` at
`p.BaseMyServiceClient.c`
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1382 ---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148808724 --- Diff: build/docker/ubuntu-trusty/Dockerfile --- @@ -221,3 +221,4 @@ ENV THRIFT_ROOT /thrift RUN mkdir -p $THRIFT_ROOT/src COPY Dockerfile $THRIFT_ROOT/ WORKDIR $THRIFT_ROOT/src + --- End diff -- My bad, will revert this as well. ---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148808675
--- Diff: lib/go/test/tests/client_error_test.go ---
@@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport,
protocol, protocol)
+ client :=
errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
Good point :) I'll add both, but it'll have to wait till tomorrow as I've
already left the office.
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148808549 --- Diff: build/docker/ubuntu-trusty/Dockerfile --- @@ -221,3 +221,4 @@ ENV THRIFT_ROOT /thrift RUN mkdir -p $THRIFT_ROOT/src COPY Dockerfile $THRIFT_ROOT/ WORKDIR $THRIFT_ROOT/src + --- End diff -- This extra line will force an image rebuild. ---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148808038
--- Diff: lib/go/test/tests/client_error_test.go ---
@@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport,
protocol, protocol)
+ client :=
errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
Not having to change the test is a better proof that the original method
works :)
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148798731 --- Diff: build/docker/ubuntu-trusty/Dockerfile --- @@ -217,6 +217,62 @@ RUN rm -rf /var/cache/apt/* && \ rm -rf /tmp/* && \ rm -rf /var/tmp/* +# Ruby --- End diff -- Fixed. ---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148797018
--- Diff: lib/go/test/tests/client_error_test.go ---
@@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport,
protocol, protocol)
+ client :=
errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
No, it's backwards compatible now (see
[here](https://github.com/dcelasun/thrift/blob/555efe5aefe9619a900471e56e86906d40bc96b9/compiler/cpp/src/thrift/generate/t_go_generator.cc#L1889-L1930)),
the test simply uses the new method.
I've also tested it with the integration of tests of a real, production
app. It works as expected.
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148796351 --- Diff: build/docker/ubuntu-trusty/Dockerfile --- @@ -217,6 +217,62 @@ RUN rm -rf /var/cache/apt/* && \ rm -rf /tmp/* && \ rm -rf /var/tmp/* +# Ruby --- End diff -- It's because I messed up the rebase. Will fix. ---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r148793389
--- Diff: lib/go/test/tests/client_error_test.go ---
@@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport,
protocol, protocol)
+ client :=
errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
Does this means the change is still not backwards compatible with
previously written customer code?
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148793005 --- Diff: build/docker/ubuntu-trusty/Dockerfile --- @@ -217,6 +217,62 @@ RUN rm -rf /var/cache/apt/* && \ rm -rf /tmp/* && \ rm -rf /var/tmp/* +# Ruby --- End diff -- This change looks incorrect. It duplicates other parts of the file. ---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1382 THRIFT-4285 Move TX/RX methods from gen. code to library This change removes a lot of duplication from generated code and allows the caller to customize how they can read from / write to the transport. This patch was originally written by [Chris Bannister](https://issues.apache.org/jira/browse/THRIFT-4285) but it seemed abandoned and no longer applied cleanly to master. I fixed it in order to get things moving again. I've also bumped `Dockerfile`s to Go 1.9 since `t.Run` in `testing/T` doesn't exist before that and we were already using 1.9 for the CentOS container. It would be great if this can be merged before 0.11 is tagged. You can merge this pull request into a Git repository by running: $ git pull https://github.com/dcelasun/thrift THRIFT-4285 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1382.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1382 ---
