From df23c8906f9e8e8ec501beee2bb79b71eb3c774c Mon Sep 17 00:00:00 2001 From: Brandon Marc-Aurele Date: Tue, 22 Oct 2024 23:38:40 -0400 Subject: [PATCH] address some self review criticism --- .../BeanBuilderAuxiliarySettersUtils.java | 55 +++++++++---------- .../java/types/BeanBuilderGenerator.java | 8 +-- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java index a4e7f9494..4e5ccd254 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java @@ -41,44 +41,43 @@ import javax.lang.model.element.Modifier; import org.apache.commons.lang3.StringUtils; -public final class BeanBuilderAuxiliarySettersUtils { +final class BeanBuilderAuxiliarySettersUtils { private BeanBuilderAuxiliarySettersUtils() {} - public static MethodSpec.Builder createPrimitiveCollectionSetterBuilder( - String prefix, - EnrichedField enriched, - TypeMapper typeMapper, - ClassName returnClass, - SafetyEvaluator safetyEvaluator) { + static MethodSpec.Builder createPrimitiveCollectionSetterBuilder( + EnrichedField enriched, TypeMapper typeMapper, ClassName returnClass, SafetyEvaluator safetyEvaluator) { FieldSpec field = enriched.poetSpec(); FieldDefinition definition = enriched.conjureDef(); - Type type = definition.getType(); - Type innerType = type.accept(TypeVisitor.LIST).getItemType(); - TypeName boxedTypeName = typeMapper.getClassName(innerType); - TypeName innerTypeName; - // SafeLong is just a special case of long - if (boxedTypeName.equals(ClassName.get(SafeLong.class))) { - innerTypeName = ConjureAnnotations.withSafety( - TypeName.LONG, safetyEvaluator.getUsageTimeSafety(enriched.conjureDef())); - } else { - innerTypeName = ConjureAnnotations.withSafety( - Primitives.unbox(boxedTypeName), safetyEvaluator.getUsageTimeSafety(enriched.conjureDef())); - } + TypeName innerTypeName = extractInnerTypeFromList(definition, typeMapper, safetyEvaluator); - return MethodSpec.methodBuilder(prefix + StringUtils.capitalize(field.name)) + return MethodSpec.methodBuilder("addAll" + StringUtils.capitalize(field.name)) .addJavadoc(Javadoc.render(definition.getDocs(), definition.getDeprecated()) .map(rendered -> CodeBlock.of("$L", rendered)) .orElseGet(() -> CodeBlock.builder().build())) .addAnnotations(ConjureAnnotations.deprecation(definition.getDeprecated())) .addModifiers(Modifier.PUBLIC) - // Var arg of the primitive type + // Forces the array argument to instead be variadic .varargs() .addParameter(Parameters.nonnullParameter(ArrayTypeName.of(innerTypeName), field.name)) .returns(returnClass); } - public static MethodSpec.Builder createCollectionSetterBuilder( + private static TypeName extractInnerTypeFromList( + FieldDefinition conjureDef, TypeMapper typeMapper, SafetyEvaluator safetyEvaluator) { + Type innerType = conjureDef.getType().accept(TypeVisitor.LIST).getItemType(); + TypeName boxedTypeName = typeMapper.getClassName(innerType); + + // SafeLong is just a special case of long + if (boxedTypeName.equals(ClassName.get(SafeLong.class))) { + return ConjureAnnotations.withSafety(TypeName.LONG, safetyEvaluator.getUsageTimeSafety(conjureDef)); + } else { + return ConjureAnnotations.withSafety( + Primitives.unbox(boxedTypeName), safetyEvaluator.getUsageTimeSafety(conjureDef)); + } + } + + static MethodSpec.Builder createCollectionSetterBuilder( String prefix, EnrichedField enriched, TypeMapper typeMapper, @@ -101,7 +100,7 @@ public static MethodSpec.Builder createCollectionSetterBuilder( field.name)); } - public static MethodSpec.Builder createOptionalSetterBuilder( + static MethodSpec.Builder createOptionalSetterBuilder( EnrichedField enriched, TypeMapper typeMapper, ClassName returnClass, SafetyEvaluator safetyEvaluator) { FieldSpec field = enriched.poetSpec(); OptionalType type = enriched.conjureDef().getType().accept(TypeVisitor.OPTIONAL); @@ -113,7 +112,7 @@ public static MethodSpec.Builder createOptionalSetterBuilder( field.name)); } - public static MethodSpec.Builder createItemSetterBuilder( + static MethodSpec.Builder createItemSetterBuilder( EnrichedField enriched, Type itemType, TypeMapper typeMapper, @@ -124,7 +123,7 @@ public static MethodSpec.Builder createItemSetterBuilder( .addParameter(ConjureAnnotations.withSafety(typeMapper.getClassName(itemType), safety), field.name); } - public static MethodSpec.Builder createMapSetterBuilder( + static MethodSpec.Builder createMapSetterBuilder( EnrichedField enriched, TypeMapper typeMapper, ClassName returnClass) { MapType type = enriched.conjureDef().getType().accept(TypeVisitor.MAP); return publicSetter(enriched, returnClass) @@ -132,7 +131,7 @@ public static MethodSpec.Builder createMapSetterBuilder( .addParameter(typeMapper.getClassName(type.getValueType()), "value"); } - public static MethodSpec.Builder publicSetter(EnrichedField enriched, ClassName returnClass) { + static MethodSpec.Builder publicSetter(EnrichedField enriched, ClassName returnClass) { FieldDefinition definition = enriched.conjureDef(); return MethodSpec.methodBuilder(enriched.poetSpec().name) .addJavadoc(Javadoc.render(definition.getDocs(), definition.getDeprecated()) @@ -143,7 +142,7 @@ public static MethodSpec.Builder publicSetter(EnrichedField enriched, ClassName .returns(returnClass); } - public static TypeName widenParameterIfPossible( + static TypeName widenParameterIfPossible( TypeName current, Type type, TypeMapper typeMapper, Optional safety) { if (type.accept(TypeVisitor.IS_LIST)) { Type innerType = type.accept(TypeVisitor.LIST).getItemType(); @@ -182,7 +181,7 @@ public static TypeName widenParameterIfPossible( // we want to widen containers of anything that's not a primitive, a conjure reference or an optional // since we know all of those are final. - public static boolean isWidenableContainedType(Type containedType) { + static boolean isWidenableContainedType(Type containedType) { return containedType.accept(new DefaultTypeVisitor() { @Override public Boolean visitPrimitive(PrimitiveType value) { diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java index bc88aca6c..d28d6f424 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java @@ -677,7 +677,7 @@ private MethodSpec createPrimitiveCollectionSetter(EnrichedField enriched, boole CollectionType collectionType = getCollectionType(type); return BeanBuilderAuxiliarySettersUtils.createPrimitiveCollectionSetterBuilder( - "addAll", enriched, typeMapper, builderClass, safetyEvaluator) + enriched, typeMapper, builderClass, safetyEvaluator) .addAnnotations(ConjureAnnotations.override(override)) .addCode(verifyNotBuilt()) .addCode(CodeBlocks.statement( @@ -689,12 +689,6 @@ private MethodSpec createPrimitiveCollectionSetter(EnrichedField enriched, boole field.name, enriched.fieldName().get() + " cannot be null"))) .addStatement("return this") .build(); - /* - ConjureCollections.class, - spec.name, - Expressions.requireNonNull( - spec.name, enriched.fieldName().get() + " cannot be null")); - */ } @SuppressWarnings("checkstyle:CyclomaticComplexity")