-
Notifications
You must be signed in to change notification settings - Fork 53
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
fix(spring): fix method javadoc missing @param and @return #1226
Conversation
componentsList.addAll(paramsList); | ||
if (!Strings.isNullOrEmpty(returnText)) { | ||
componentsList.add(String.format("@return %s", returnText)); |
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.
This is great, and will be a big win for for the main branch. Any chance we could get this implemented in main, then merge the update from main into autoconfig-gen-draft2? (It may not be that simple, though -- since it looks like this branch still has to sync with the multimodule update.)
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.
Shouldn't this cause a large scale change in the golden files, or am I misunderstanding the scope of this change?
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.
@burkedavison with the multimodule update, it's not too feasible to merge main
into autoconfig-gen-draft2
(which will always stay as a separate branch) - but copying the relevant changes in a separate PR to main
might be the easier way to go here?
(For context: have a similar commit in this situation as part of #1208 - given that spring codegen plans to eventually move to use the gapic-generator-java jar, these changes do also need to be made in main
; we are adding them directly to the spring branch for now given the current repo structure divergence.)
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.
This change is enabling us to add @return
to the JavaDocs. You would still need to set returnText
in the composers class for each method to actually generate the JavaDocs.
@burkedavison We decided that the SpringCodeGen will continue to work on a branch until preview release to prevent disruptions. That being said, I agree this change is important to the main branch as well and we should backport it, @zhumin8 can you please add it to the TODO list? We can create a PR against main with just this change.
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.
Thanks for clarifying!
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.
Will do!
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.
Nice!
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.
One small test comment fix, otherwise LGTM!
src/test/java/com/google/api/generator/engine/ast/JavaDocCommentTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Emily Wang <emmwang@google.com>
Kudos, SonarCloud Quality Gate passed! |
Adds
@param
and@return
to method javadoc comments in generated autoconfiguration classes.Also adding a change to
JavaDocComment
to enable@return
in comment. (this change in 537e33c can be copied to main branch ofgapic-generator-java
if useful.)