ctubbsii commented on code in PR #3017:
URL: https://github.com/apache/thrift/pull/3017#discussion_r1725458390
##########
compiler/cpp/src/thrift/generate/t_java_generator.cc:
##########
@@ -3328,6 +3328,7 @@ void t_java_generator::generate_service_client(t_service*
tservice) {
// Careful, only return _result if not a void function
if (!(*f_iter)->get_returntype()->is_void()) {
f_service_ << indent() << "if (result." <<
generate_isset_check("success") << ") {" << '\n'
+ << indent() << " after();" << '\n'
Review Comment:
You're adding an extensible method to the abstract base class called "after"
without any javadoc to explain when it's called, or any more specific name that
answers the question: "after what?"
Then, you're calling it in a very specific scenario in the generated code...
only after calling a non-void method that has a response. So, even though
you've written it to be extensible, you're not calling it in an extensible way.
If the method is overridden to do other things, then it should be called in
other situations, so it can do those other things.
So, you should either:
1. Write it to be extensible as a lifecycle method and call it in more
scenarios, with a javadoc to explain the lifecycle ("after what?"), or
2. You should narrow its purpose so it's not intended to be extensible as
part of a lifecycle and give it a much more targeted name like
`clearTransportBuffer()` to describe it's function rather than its lifecycle
phase, and only call it in the case you need to.
Also, please stop force-pushing while it's under review. It's making it
nearly impossible to review the diffs between each change, if there are any.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@thrift.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
