-
Notifications
You must be signed in to change notification settings - Fork 4.1k
THRIFT-5551: java generator to add @Override where possible #2559
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
Conversation
ctubbsii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are harder to review than necessary, because they contain a lot of style changes. I would recommend minimizing the changes in this PR by retaining the existing style, so it's easier to review. For style standardization/improvements, I'd suggest creating a separate PR that does only that and no substantive changes.
Thanks for the suggestion - I wonder it too. The formatting changes were made by my editor, and I don't have a good way to separate it from the actual changes now. If it's okay do you think I can create a separate, clean, style only change and merge that first. And then this change can be much cleaner? @ctubbsii |
Yeah, I think that would be a good option. If a standard style can be enforced by people with any editor, something like what the formatter-maven-plugin provides for Java code, that would be even better, I would think. |
|
yes @ctubbsii the tool is
|
|
I just learned about clang-format. It seems nice... but a quick check shows it's defaults would change a lot of thrift code, and not always for the better. I'm not sure if the Thrift community would endorse its use, but there is a .clang-format file in the root directory. It seems it hasn't been run in awhile. It might be easier to just look over this PR and try to undo the formatting changes. Or, just rely on reviewers to figure it out. |
c8cf896 to
46aaed2
Compare
|
@jimexist I think you need to rebase and force-push to get this up-to-date. |
46aaed2 to
806886b
Compare
ctubbsii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks pretty good to me. I haven't verified that all the new uses are actually overrides, but it will be pretty obvious if they aren't... it will generate code that won't compile if they aren't.
Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>
|
|
||
| void t_java_generator::generate_java_struct_clear(std::ostream& out, t_struct* tstruct) { | ||
| if (!java5_) { | ||
| indent(out) << "@Override" << endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Override is _since_ 5.0
| indent(f_service_) << " this.clientManager = clientManager;" << endl; | ||
| indent(f_service_) << " this.protocolFactory = protocolFactory;" << endl; | ||
| indent(f_service_) << " }" << endl; | ||
| indent(f_service_) << " @Override" << endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a test, right? Seen and fixed.
java generator to add @OverRide where possible
[skip ci]anywhere in the commit message to free up build resources.