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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 10, 2023
Merged

fix(spring): fix method javadoc missing @param and @return #1226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
537e33c
fix(ast): update JavaDocComment to support @return.
zhumin8 Jan 6, 2023
d2b9543
add @param and @return to method javadoc in generated autoconfig clas…
zhumin8 Jan 6, 2023
de04996
minor changes in comment.
zhumin8 Jan 6, 2023
3cc6b0d
remove irrelevant comments.
zhumin8 Jan 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/main/java/com/google/api/generator/engine/ast/JavaDocComment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public abstract static class Builder {
String throwsDescription = null;
String deprecated = null;
List<String> paramsList = new ArrayList<>();
String returnText = null;
List<String> componentsList = new ArrayList<>();
// Private accessor, set complete and consolidated comment.
abstract Builder setComment(String comment);
Expand All @@ -69,6 +70,11 @@ public Builder setDeprecated(String deprecatedText) {
return this;
}

public Builder setReturn(String returnDescription) {
returnText = returnDescription;
return this;
}

public Builder addParam(String name, String description) {
paramsList.add(String.format("@param %s %s", name, processParamComment(description)));
return this;
Expand Down Expand Up @@ -130,12 +136,16 @@ public boolean emptyComments() {
&& Strings.isNullOrEmpty(throwsDescription)
&& Strings.isNullOrEmpty(deprecated)
&& paramsList.isEmpty()
&& Strings.isNullOrEmpty(returnText)
&& componentsList.isEmpty();
}

public JavaDocComment build() {
// @param, @throws and @deprecated should always get printed at the end.
// @param, @return, @throws and @deprecated should always get printed at the end.
componentsList.addAll(paramsList);
if (!Strings.isNullOrEmpty(returnText)) {
componentsList.add(String.format("@return %s", returnText));
Copy link
Member

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.)

Copy link
Member

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?

Copy link
Contributor

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.)

Copy link
Collaborator

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.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

}
if (!Strings.isNullOrEmpty(throwsType)) {
componentsList.add(
String.format("@throws %s %s", throwsType, HtmlEscaper.process(throwsDescription)));
Expand Down
18 changes: 17 additions & 1 deletion ...ava/com/google/api/generator/spring/composer/comment/SpringAutoconfigCommentComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@ public class SpringAutoconfigCommentComposer {
+ "configuration data files.";

public static final String TRANSPORT_CHANNEL_PROVIDER_GENERAL_DESCRIPTION =
"Returns the default channel provider. The default is gRPC and will default to it unless the "
"Provides a default transport channel provider bean. The default is gRPC and will default to it unless the "
+ "useRest option is provided to use HTTP transport instead";
public static final String TRANSPORT_CHANNEL_PROVIDER_RETURN =
"a default transport channel provider.";
public static final String CLIENT_SETTINGS_BEAN_GENERAL_DESCRIPTION =
"Provides a %sSettings bean configured to "
+ "use the default credentials provider (obtained with %sCredentials()) and its default "
Expand All @@ -54,9 +56,13 @@ public class SpringAutoconfigCommentComposer {
"Retry settings are also configured from service-level and method-level properties specified in %s. "
+ "Method-level properties will take precedence over service-level properties if available, "
+ "and client library defaults will be used if neither are specified.";
public static final String CLIENT_SETTINGS_BEAN_RETURN_STATEMENT =
"a {@link %sSettings} bean configured with {@link TransportChannelProvider} bean.";

public static final String CLIENT_BEAN_GENERAL_DESCRIPTION =
"Provides a %sClient bean configured with %sSettings.";
public static final String CLIENT_BEAN_RETURN_STATEMENT =
"a {@link %sClient} bean configured with {@link %sSettings}";

public SpringAutoconfigCommentComposer() {}

Expand Down Expand Up @@ -88,6 +94,7 @@ public static CommentStatement createTransportChannelProviderComment() {
return CommentStatement.withComment(
JavaDocComment.builder()
.addParagraph(TRANSPORT_CHANNEL_PROVIDER_GENERAL_DESCRIPTION)
.setReturn(TRANSPORT_CHANNEL_PROVIDER_RETURN)
.build());
}

Expand All @@ -104,13 +111,22 @@ public static CommentStatement createSettingsBeanComment(
channelProviderName))
.addParagraph(
String.format(CLIENT_SETTINGS_BEAN_RETRY_SETTINGS_DESCRIPTION, propertiesClazzName))
.addParam(
"defaultTransportChannelProvider",
"TransportChannelProvider to use in the settings.")
.setReturn(String.format(CLIENT_SETTINGS_BEAN_RETURN_STATEMENT, serviceName))
.build());
}

public static CommentStatement createClientBeanComment(String serviceName) {
String lowerServiceName = CaseFormat.UPPER_CAMEL.to(CaseFormat.LOWER_CAMEL, serviceName);
return CommentStatement.withComment(
JavaDocComment.builder()
.addParagraph(String.format(CLIENT_BEAN_GENERAL_DESCRIPTION, serviceName, serviceName))
.addParam(
String.format("%sSettings", lowerServiceName),
"settings to configure an instance of client bean.")
.setReturn(String.format(CLIENT_BEAN_RETURN_STATEMENT, serviceName, serviceName))
.build());
}
}
25 changes: 25 additions & 0 deletions src/test/java/com/google/api/generator/engine/ast/JavaDocCommentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,28 @@ public void createJavaDocComment_throwsAndDeprecated() {
assertEquals(expected, javaDocComment.comment());
}

@Test
public void createJavaDocComment_paramsAndReturn() {
String paramName1 = "shelfName";
String paramDescription1 = "The name of the shelf where books are published to.";
String paramName2 = "shelf";
String paramDescription2 = "The shelf to create.";
String returnText = "This is the method return text.";

JavaDocComment javaDocComment =
JavaDocComment.builder()
.addParam(paramName1, paramDescription1)
.addParam(paramName2, paramDescription2)
.setReturn(returnText)
.build();
String expected =
LineFormatter.lines(
"@param shelfName The name of the shelf where books are published to.\n",
"@param shelf The shelf to create.\n",
"@return This is the method return text.");
assertEquals(expected, javaDocComment.comment());
}

@Test
public void createJavaDocComment_allComponents() {
// No matter what order `setThrows`, `setDeprecated` are called,
Expand All @@ -190,6 +212,7 @@ public void createJavaDocComment_allComponents() {
String paramDescription1 = "The name of the shelf where books are published to.";
String paramName2 = "shelf";
String paramDescription2 = "The shelf to create.";
String returnText = "This is the method return text.";
String paragraph1 =
"This class provides the ability to make remote calls to the backing service through"
+ " method calls that map to API methods. Sample code to get started:";
Expand All @@ -210,6 +233,7 @@ public void createJavaDocComment_allComponents() {
.addParagraph(paragraph2)
.addOrderedList(orderedList)
.addParam(paramName2, paramDescription2)
.setReturn(returnText)
.build();
String expected =
LineFormatter.lines(
Expand All @@ -225,6 +249,7 @@ public void createJavaDocComment_allComponents() {
"</ol>\n",
"@param shelfName The name of the shelf where books are published to.\n",
"@param shelf The shelf to create.\n",
"@return This is the method return text.\n",
"@throws com.google.api.gax.rpc.ApiException if the remote call fails.\n",
"@deprecated Use the {@link ArchivedBookName} class instead.");
assertEquals(expected, javaDocComment.comment());
Expand Down
16 changes: 13 additions & 3 deletions ...a/com/google/api/generator/spring/composer/goldens/EchoSpringAutoConfigurationFull.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,10 @@ public class EchoSpringAutoConfiguration {
}

/**
* Returns the default channel provider. The default is gRPC and will default to it unless the
* useRest option is provided to use HTTP transport instead
* Provides a default transport channel provider bean. The default is gRPC and will default to it
* unless the useRest option is provided to use HTTP transport instead
*
* @return a default transport channel provider.
*/
@Bean
@ConditionalOnMissingBean(name = "defaultEchoTransportChannelProvider")
Expand All @@ -104,6 +106,9 @@ public class EchoSpringAutoConfiguration {
* <p>Retry settings are also configured from service-level and method-level properties specified
* in EchoSpringProperties. Method-level properties will take precedence over service-level
* properties if available, and client library defaults will be used if neither are specified.
*
* @param defaultTransportChannelProvider TransportChannelProvider to use in the settings.
* @return a {@link EchoSettings} bean configured with {@link TransportChannelProvider} bean.
*/
@Bean
@ConditionalOnMissingBean
Expand Down Expand Up @@ -302,7 +307,12 @@ public class EchoSpringAutoConfiguration {
return clientSettingsBuilder.build();
}

/** Provides a EchoClient bean configured with EchoSettings. */
/**
* Provides a EchoClient bean configured with EchoSettings.
*
* @param echoSettings settings to configure an instance of client bean.
* @return a {@link EchoClient} bean configured with {@link EchoSettings}
*/
@Bean
@ConditionalOnMissingBean
public EchoClient echoClient(EchoSettings echoSettings) throws IOException {
Expand Down
16 changes: 13 additions & 3 deletions ...a/com/google/api/generator/spring/composer/goldens/EchoSpringAutoConfigurationGrpc.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,10 @@ public class EchoSpringAutoConfiguration {
}

/**
* Returns the default channel provider. The default is gRPC and will default to it unless the
* useRest option is provided to use HTTP transport instead
* Provides a default transport channel provider bean. The default is gRPC and will default to it
* unless the useRest option is provided to use HTTP transport instead
*
* @return a default transport channel provider.
*/
@Bean
@ConditionalOnMissingBean(name = "defaultEchoTransportChannelProvider")
Expand All @@ -84,6 +86,9 @@ public class EchoSpringAutoConfiguration {
* <p>Retry settings are also configured from service-level and method-level properties specified
* in EchoSpringProperties. Method-level properties will take precedence over service-level
* properties if available, and client library defaults will be used if neither are specified.
*
* @param defaultTransportChannelProvider TransportChannelProvider to use in the settings.
* @return a {@link EchoSettings} bean configured with {@link TransportChannelProvider} bean.
*/
@Bean
@ConditionalOnMissingBean
Expand Down Expand Up @@ -282,7 +287,12 @@ public class EchoSpringAutoConfiguration {
return clientSettingsBuilder.build();
}

/** Provides a EchoClient bean configured with EchoSettings. */
/**
* Provides a EchoClient bean configured with EchoSettings.
*
* @param echoSettings settings to configure an instance of client bean.
* @return a {@link EchoClient} bean configured with {@link EchoSettings}
*/
@Bean
@ConditionalOnMissingBean
public EchoClient echoClient(EchoSettings echoSettings) throws IOException {
Expand Down
16 changes: 13 additions & 3 deletions ...m/google/api/generator/spring/composer/goldens/EchoSpringAutoConfigurationGrpcRest.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,10 @@ public class EchoSpringAutoConfiguration {
}

/**
* Returns the default channel provider. The default is gRPC and will default to it unless the
* useRest option is provided to use HTTP transport instead
* Provides a default transport channel provider bean. The default is gRPC and will default to it
* unless the useRest option is provided to use HTTP transport instead
*
* @return a default transport channel provider.
*/
@Bean
@ConditionalOnMissingBean(name = "defaultEchoTransportChannelProvider")
Expand All @@ -85,6 +87,9 @@ public class EchoSpringAutoConfiguration {
* <p>Retry settings are also configured from service-level and method-level properties specified
* in EchoSpringProperties. Method-level properties will take precedence over service-level
* properties if available, and client library defaults will be used if neither are specified.
*
* @param defaultTransportChannelProvider TransportChannelProvider to use in the settings.
* @return a {@link EchoSettings} bean configured with {@link TransportChannelProvider} bean.
*/
@Bean
@ConditionalOnMissingBean
Expand Down Expand Up @@ -290,7 +295,12 @@ public class EchoSpringAutoConfiguration {
return clientSettingsBuilder.build();
}

/** Provides a EchoClient bean configured with EchoSettings. */
/**
* Provides a EchoClient bean configured with EchoSettings.
*
* @param echoSettings settings to configure an instance of client bean.
* @return a {@link EchoClient} bean configured with {@link EchoSettings}
*/
@Bean
@ConditionalOnMissingBean
public EchoClient echoClient(EchoSettings echoSettings) throws IOException {
Expand Down