Return enums from model getters and make enums follow the standard java naming scheme. #195

millems · 2017-10-06T23:26:52Z

Where appropriate, enums are now returned from model getters instead of
strings, which have been moved to a separate method that indicates its
value is a string.

Generated enums now follow the SCREAMING_CASE convention instead of the
PascalCase that was previously used.

spfink · 2017-10-09T18:31:47Z

codegen/src/test/java/software/amazon/awssdk/codegen/naming/DefaultNamingStrategyTest.java

+        validateConversion("TLSv1.2", "TLS_V1_2");
+        validateConversion("us-east-1", "US_EAST_1");
+        validateConversion("io1", "IO1");
+        validateConversion("application/vnd.amazonaws.card.generic", "APPLICATION_VND_AMAZONAWS_CARD_GENERIC");


Duplicated from line 49

Good catch!

spfink · 2017-10-09T18:51:24Z

codegen/src/test/resources/software/amazon/awssdk/codegen/poet/common/test-enum-class.java

     *
     * @param value
     *        real value
     * @return TestEnumClass corresponding to the value
     */
    public static TestEnumClass fromValue(String value) {
-        if (isBlank(value)) {
-            throw new IllegalArgumentException("Value cannot be null or empty!");
+        if (value == null) {


Why the change in behavior here?

It was easier to use this way. If the service returned null, you'd have to do a null and isEmpty check before converting it to an enum. This way, a null response from the service is a null enum, and an empty-string response from the service is an UNKNOWN enum.

shorea · 2017-10-11T22:20:46Z

codegen/src/test/resources/software/amazon/awssdk/codegen/poet/model/alltypesrequest.java

@@ -307,11 +448,29 @@ public SubTypeOne polymorphicTypeWithoutSubTypes() {

    /**
     * Returns the value of the EnumType property for this object.
+     * <p>
+     * If the service returns an enum value that is not available in the current SDK version, {@link #enumType} will
+     * return {@link EnumType#UNKNOWN}. The raw value returned by the service is available from {@link #enumTypeString}.


We should use SDK_UNKNOWN or something like that to prevent clash with service define values

https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-models/src/main/resources/models/discovery-2015-11-01-model.json#L425

shorea · 2017-10-11T22:22:10Z

codegen/src/test/resources/software/amazon/awssdk/codegen/poet/model/alltypesrequest.java

+         *        The new value for the ListOfEnums property for this object.
+         * @return Returns a reference to this object so that method calls can be chained together.
+         */
+        Builder listOfEnums(String... listOfEnums);


Why can't you set values via the enum?

We'll need to add support for that. We just don't have it yet, and I didn't want to make this PR any longer.

shorea · 2017-10-11T22:28:36Z

test/protocol-tests/src/test/java/software/amazon/awssdk/protocol/tests/EnumTest.java

+                .containsExactly(new SimpleImmutableEntry<>(EnumType.ENUM_VALUE1, EnumType.UNKNOWN));
+    }
+
+    private EnumType simulateUnmarshallingEnum(String value) {


Seems kind of a weird name. You're really just testing the builder behavior here.

I'm doing what the unmarshaller does. I'll add a comment.

shorea · 2017-10-11T22:33:40Z

codegen/src/main/java/software/amazon/awssdk/codegen/naming/DefaultNamingStrategy.java

-        String sanitizedEnumValue = enumValue.replace("::", ":").replace("/", "").replace("(", "")
-                                             .replace(")", "");
+        // Special cases
+        result = result.replaceAll("IoT", "IOT")


Should we treat these as customizations rather then put them in Core?

Do we have the feature in place to customize enum names? I didn't see anything like that.

I'm not sure, but we can add one. In general we try not to put service specific customizations in the code gen.

That's fair - it's not good to have service-specific stuff in the generator.

IoT in this case isn't service-specific. I'm not aware of any service using it in their enums, but they may in the future and we'd probably not notice it (generating IO_T instead).

It's just textORcsv that is service-specific in this PR. Because it's just this one example, I'd vote we follow the rule-of-three and add a customization when we have more cases in which we would use it.

I don't think it is likely that more cases like this are going to happen. AWS service owners are warned now if their enums don't follow SCREAMING_CASE conventions. If something does happen to get through review: (a) we'll catch it before it's released and have the service owner fix their model, or (b) we won't catch it before it's released and at that point we can't change it anyway.

Should the IoT case just be baked into the logic then. I.E. is there any value where we'd want to separate a single capital letter via underscore? If not can we just treat that as part of the previous word boundary?

Sounds good to me.

shorea · 2017-10-11T22:38:32Z

codegen/src/main/java/software/amazon/awssdk/codegen/naming/DefaultNamingStrategy.java

+
+        if (!result.matches("^[A-Z][A-Z0-9_]*$")) {
+            String attempt = result;
+            log.warn(() -> "Invalid enum member generated for input '" + enumValue + "'. Best attempt: '" + attempt + "' If this "


Why not kill it here?

We have customizations that remove these values in a later part of code generation.

shorea · 2017-10-11T22:40:24Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/AwsServiceModel.java

+            result.add(enumMemberGetter(member));
+        }
+
+        result.add(nonEnumMemberGetter(member));


Name is kind of unintuitive as you'd call it for enums too it would just emit the string one.

I'm trying to say "generate the non-enum getter for this member". For enums, that's a string. What name would you like me to use instead?

I'd just call it memberGetter since it's the primary getter for any type of field, the enum one is the convenience one.

naming scheme. Where appropriate, enums are now returned from model getters instead of strings, which have been moved to a separate method that indicates its value is a string. Generated enums now follow the SCREAMING_CASE convention instead of the PascalCase that was previously used.

millems force-pushed the millem/enums branch 2 times, most recently from 475c2af to 1a8aea6 Compare October 9, 2017 17:37

spfink reviewed Oct 9, 2017

View reviewed changes

millems force-pushed the millem/enums branch 3 times, most recently from ab2faeb to 7ee7b13 Compare October 9, 2017 22:02

shorea reviewed Oct 11, 2017

View reviewed changes

millems force-pushed the millem/enums branch from f156d78 to 5db9cde Compare October 13, 2017 19:09

shorea approved these changes Oct 16, 2017

View reviewed changes

millems force-pushed the millem/enums branch from 95e53f7 to b773bef Compare October 16, 2017 18:45

millems merged commit f2a8b9b into master Oct 16, 2017

millems deleted the millem/enums branch October 16, 2017 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return enums from model getters and make enums follow the standard java naming scheme. #195

Return enums from model getters and make enums follow the standard java naming scheme. #195

millems commented Oct 6, 2017

spfink Oct 9, 2017

millems Oct 9, 2017

spfink Oct 9, 2017

millems Oct 9, 2017

shorea Oct 11, 2017

millems Oct 11, 2017

shorea Oct 11, 2017

millems Oct 11, 2017

shorea Oct 11, 2017

millems Oct 11, 2017

shorea Oct 11, 2017

millems Oct 11, 2017

shorea Oct 13, 2017

millems Oct 13, 2017

shorea Oct 13, 2017

millems Oct 13, 2017

shorea Oct 11, 2017

millems Oct 11, 2017

shorea Oct 11, 2017

millems Oct 11, 2017

shorea Oct 13, 2017

millems Oct 13, 2017

Return enums from model getters and make enums follow the standard java naming scheme. #195

Return enums from model getters and make enums follow the standard java naming scheme. #195

Conversation

millems commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment