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

Jump to bottom

fix: eliminate raw-types and remove unnecessary cast #1095

Merged
merged 7 commits into from
Nov 18, 2022
Merged

fix: eliminate raw-types and remove unnecessary cast #1095

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e75338
fix(deps): update dependency com.google.cloud:google-cloud-shared-dep…
renovate-bot Nov 8, 2022
ee5d7a5
chore(deps): update dependency kr.motd.maven:os-maven-plugin to v1.7.…
renovate-bot Nov 8, 2022
4fd1f98
fix: eliminate raw-types and remove unnecessary cast
burkedavison Nov 14, 2022
29ca198
fix(spring): eliminate raw-types and remove unnecessary cast
burkedavison Nov 14, 2022
c83a887
Merge remote-tracking branch 'origin/fix-raw-types' into fix-raw-types
burkedavison Nov 14, 2022
49a3ad9
Merge branch 'main' into fix-raw-types
burkedavison Nov 17, 2022
51dbc29
Merge branch 'autoconfig-gen-draft2' into fix-raw-types
burkedavison Nov 17, 2022
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
5 changes: 3 additions & 2 deletions src/main/java/com/google/api/generator/spring/composer/SpringAutoConfigClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import java.util.stream.Collectors;

public class SpringAutoConfigClassComposer implements ClassComposer {

private static final String CLASS_NAME_PATTERN = "%sSpringAutoConfiguration";

private static final SpringAutoConfigClassComposer INSTANCE = new SpringAutoConfigClassComposer();
Expand Down Expand Up @@ -665,7 +666,7 @@ private static MethodDefinition createClientBeanMethod(
}
// retry settings for each method
TypeNode thisClassType = types.get(service.name() + "AutoConfig");
List retrySettings =
List<Statement> retrySettings =
Utils.processRetrySettings(
service,
gapicServiceConfig,
Expand Down Expand Up @@ -876,7 +877,7 @@ private static MethodDefinition createUserAgentHeaderProviderMethod(
}

private static Map<String, TypeNode> createStaticTypes() {
List<Class> concreteClazzes =
List<Class<?>> concreteClazzes =
Arrays.asList(
RetrySettings.class,
RetrySettings.Builder
Expand Down
13 changes: 6 additions & 7 deletions src/main/java/com/google/api/generator/spring/composer/SpringPropertiesClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,11 @@

package com.google.api.generator.spring.composer;

import static com.google.api.generator.engine.ast.NewObjectExpr.*;
import static com.google.api.generator.engine.ast.NewObjectExpr.builder;

import com.google.api.gax.retrying.RetrySettings;
import com.google.api.generator.engine.ast.AnnotationNode;
import com.google.api.generator.engine.ast.AssignmentExpr;
import com.google.api.generator.engine.ast.AstNode;
import com.google.api.generator.engine.ast.ClassDefinition;
import com.google.api.generator.engine.ast.ConcreteReference;
import com.google.api.generator.engine.ast.Expr;
Expand Down Expand Up @@ -55,6 +54,7 @@
import java.util.stream.Collectors;

public class SpringPropertiesClassComposer implements ClassComposer {

private static final String CLASS_NAME_PATTERN = "%sSpringProperties";

private static final Map<String, TypeNode> staticTypes = createStaticTypes();
Expand Down Expand Up @@ -187,7 +187,7 @@ private static List<Statement> createMemberVariables(

// declare each retry settings with its default value. use defaults from serviceConfig
TypeNode thisClassType = types.get(service.name() + "Properties");
List<? extends AstNode> retrySettings =
List<Statement> retrySettings =
Utils.processRetrySettings(
service,
serviceConfig,
Expand All @@ -208,8 +208,7 @@ private static List<Statement> createMemberVariables(
},
(String propertyName) -> new ArrayList<>());

statements.addAll(
retrySettings.stream().map(Statement.class::cast).collect(Collectors.toList()));
statements.addAll(retrySettings);

return statements;
}
Expand Down Expand Up @@ -241,7 +240,7 @@ private static List<MethodDefinition> createGetterSetters(
methodDefinitions.add(
createSetterMethod(thisClassType, "executorThreadCount", TypeNode.INT_OBJECT));

List retrySettings =
List<MethodDefinition> retrySettings =
Utils.processRetrySettings(
service,
gapicServiceConfig,
Expand Down Expand Up @@ -381,7 +380,7 @@ private static Map<String, TypeNode> createDynamicTypes(Service service, String
}

private static Map<String, TypeNode> createStaticTypes() {
List<Class> concreteClazzes =
List<Class<?>> concreteClazzes =
Arrays.asList(RetrySettings.class, org.threeten.bp.Duration.class);
return concreteClazzes.stream()
.collect(
Expand Down
11 changes: 6 additions & 5 deletions src/main/java/com/google/api/generator/spring/utils/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.function.Function;

public class Utils {

private static final TypeStore FIXED_TYPESTORE = createStaticTypes();

private static final String BRAND_NAME = "spring.cloud.gcp";
Expand Down Expand Up @@ -90,14 +91,14 @@ public static String getSpringPropertyPrefix(String packageName, String serviceN
+ CaseFormat.UPPER_CAMEL.to(CaseFormat.LOWER_HYPHEN, serviceName);
}

public static List<? extends AstNode> processRetrySettings(
public static <T extends AstNode> List<T> processRetrySettings(
Service service,
GapicServiceConfig gapicServiceConfig,
TypeNode thisClassType,
Function<String, List<? extends AstNode>> perMethodFuncBeforeSettings,
BiFunction<List<String>, Expr, List<? extends AstNode>> processFunc,
Function<String, List<? extends AstNode>> perMethodFuncAfterSettings) {
List resultList = new ArrayList<>();
Function<String, List<? extends T>> perMethodFuncBeforeSettings,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we change <? extends T> to T as well? Looks like what gets returned are always the same type?

Copy link
Contributor Author

@burkedavison burkedavison Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

T is valid for the current source; but as a Util function, the limit of the implementation is <? extends T> which defines how it can be used in the future as well. This also decouples future functions from needing to return the same type of AstNode list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, without the ? extends T, it wouldn't be possible to combine type-related-but-not-the-same helper functions:

abstract List<CommentStatement> getHeaderComments(String methodName);
abstract List<Statement> getStatements(String methodName);
private static <T> List<T> NONE(String unused) { return Collections.emptyList(); }

private void example() {
  List<Statement> statements = Util.processRetrySettings(
    server,
    config,
    clazzType,
    this::getHeaderComments,
    this::getStatements,
    this::NONE
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the general idea. But for this util method, it's a very specific use case(Create retry settings with some hardcoded method names) that is only used by the two composers, so I doubt it will be used by anything else. I could also argue that this method should not be a public Util, maybe it should be in something like a AbstractSpringComposer with default scope.
That being said, I don't think this is critical enough to block the PR, so either way works.

BiFunction<List<String>, Expr, List<? extends T>> processFunc,
Function<String, List<? extends T>> perMethodFuncAfterSettings) {
List<T> resultList = new ArrayList<>();
for (Method method : service.methods()) {
String methodName = CaseFormat.UPPER_CAMEL.to(CaseFormat.LOWER_CAMEL, method.name());
String retryParamName = gapicServiceConfig.getRetryParamsName(service, method);
Expand Down