Fix: Unions now consistently reference field names (#221)

Unions now consistently reference and initialize their field names.
palantir · Oct 25, 2019 · ba4d768 · ba4d768
1 parent c6dbc5c
commit ba4d768
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 21 deletions.
diff --git a/changelog/@unreleased/pr-221.v2.yml b/changelog/@unreleased/pr-221.v2.yml
@@ -0,0 +1,5 @@
+type: fix
+fix:
+  description: Unions now consistently reference and initialize their field names.
+  links:
+  - https://github.com/palantir/conjure-python/pull/221
diff --git a/conjure-python-core/src/main/java/com/palantir/conjure/python/poet/UnionSnippet.java b/conjure-python-core/src/main/java/com/palantir/conjure/python/poet/UnionSnippet.java
@@ -56,6 +56,26 @@ default String idForSorting() {
 
     List<PythonField> options();
 
+    /** The name of the option as a constructor / method parameter. */
+    static String parameterName(PythonField option) {
+        return PythonIdentifierSanitizer.sanitize(option.attributeName());
+    }
+
+    /** The name of the option as a {@code @property}. Must be different from {@link #fieldName}. */
+    static String propertyName(PythonField option) {
+        return PythonIdentifierSanitizer.sanitize(option.attributeName());
+    }
+
+    /** The name of the option as a field. */
+    static String fieldName(PythonField option) {
+        return "_" + PythonIdentifierSanitizer.sanitize(option.attributeName(), PROTECTED_FIELDS);
+    }
+
+    /** The name of the visitor method for the given option. */
+    static String visitorMethodName(PythonField option) {
+        return "_" + option.attributeName();
+    }
+
     @Override
     default void emit(PythonPoetWriter poetWriter) {
         poetWriter.writeIndentedLine(String.format("class %s(ConjureUnionType):", className()));
@@ -64,8 +84,8 @@ default void emit(PythonPoetWriter poetWriter) {
 
         poetWriter.writeLine();
 
-        options().forEach(option -> poetWriter.writeIndentedLine("_%s = None # type: %s",
-                option.attributeName(), option.myPyType()));
+        options().forEach(option -> poetWriter.writeIndentedLine("%s = None # type: %s",
+                fieldName(option), option.myPyType()));
 
         poetWriter.writeLine();
 
@@ -79,7 +99,7 @@ default void emit(PythonPoetWriter poetWriter) {
         for (int i = 0; i < options().size(); i++) {
             PythonField option = options().get(i);
             poetWriter.writeIndentedLine("'%s': ConjureFieldDefinition('%s', %s)%s",
-                    PythonIdentifierSanitizer.sanitize(option.attributeName()),
+                    propertyName(option),
                     option.jsonIdentifier(),
                     option.pythonType(),
                     i == options().size() - 1 ? "" : ",");
@@ -103,8 +123,7 @@ default void emit(PythonPoetWriter poetWriter) {
         poetWriter.writeIndentedLine("if %s != 1:",
                 Joiner.on(" + ").join(
                     options().stream()
-                        .map(option -> String.format("(%s is not None)",
-                                PythonIdentifierSanitizer.sanitize(option.attributeName())))
+                        .map(option -> String.format("(%s is not None)", parameterName(option)))
                         .collect(Collectors.toList())));
         poetWriter.increaseIndent();
         poetWriter.writeIndentedLine("raise ValueError('a union must contain a single member')");
@@ -113,12 +132,9 @@ default void emit(PythonPoetWriter poetWriter) {
         poetWriter.writeLine();
         // save off
         options().forEach(option -> {
-            poetWriter.writeIndentedLine("if %s is not None:",
-                    PythonIdentifierSanitizer.sanitize(option.attributeName()));
+            poetWriter.writeIndentedLine("if %s is not None:", parameterName(option));
             poetWriter.increaseIndent();
-            poetWriter.writeIndentedLine("self._%s = %s",
-                    PythonIdentifierSanitizer.sanitize(option.attributeName(), PROTECTED_FIELDS),
-                    PythonIdentifierSanitizer.sanitize(option.attributeName()));
+            poetWriter.writeIndentedLine("self.%s = %s", fieldName(option), parameterName(option));
             poetWriter.writeIndentedLine("self._type = '%s'", option.jsonIdentifier());
             poetWriter.decreaseIndent();
         });
@@ -128,15 +144,13 @@ default void emit(PythonPoetWriter poetWriter) {
         options().forEach(option -> {
             poetWriter.writeLine();
             poetWriter.writeIndentedLine("@property");
-            poetWriter.writeIndentedLine(String.format("def %s(self):",
-                    PythonIdentifierSanitizer.sanitize(option.attributeName())));
+            poetWriter.writeIndentedLine(String.format("def %s(self):", propertyName(option)));
 
             poetWriter.increaseIndent();
             poetWriter.writeIndentedLine(String.format("# type: () -> %s", option.myPyType()));
             option.docs().ifPresent(docs -> poetWriter.writeIndentedLine(String.format("\"\"\"%s\"\"\"",
                     docs.get().trim())));
-            poetWriter.writeIndentedLine(String.format("return self._%s",
-                    PythonIdentifierSanitizer.sanitize(option.attributeName(), PROTECTED_FIELDS)));
+            poetWriter.writeIndentedLine(String.format("return self.%s", fieldName(option)));
             poetWriter.decreaseIndent();
         });
 
@@ -155,8 +169,7 @@ default void emit(PythonPoetWriter poetWriter) {
         options().forEach(option -> {
             poetWriter.writeIndentedLine("if self.type == '%s':", option.jsonIdentifier());
             poetWriter.increaseIndent();
-            poetWriter.writeIndentedLine("return visitor._%s(self.%s)", option.attributeName(),
-                    PythonIdentifierSanitizer.sanitize(option.attributeName()));
+            poetWriter.writeIndentedLine("return visitor.%s(self.%s)", visitorMethodName(option), propertyName(option));
             poetWriter.decreaseIndent();
         });
         poetWriter.decreaseIndent();
@@ -176,8 +189,7 @@ default void emit(PythonPoetWriter poetWriter) {
         options().forEach(option -> {
             poetWriter.writeLine();
             poetWriter.writeIndentedLine("@abstractmethod");
-            poetWriter.writeIndentedLine("def _%s(self, %s):", option.attributeName(),
-                    PythonIdentifierSanitizer.sanitize(option.attributeName()));
+            poetWriter.writeIndentedLine("def %s(self, %s):", visitorMethodName(option), parameterName(option));
             poetWriter.increaseIndent();
             poetWriter.writeIndentedLine("# type: (%s) -> Any", option.myPyType());
             poetWriter.writeIndentedLine("pass");

diff --git a/conjure-python-core/src/test/resources/types/example-types.yml b/conjure-python-core/src/test/resources/types/example-types.yml
@@ -1,4 +1,3 @@
-# don't forget to update conjure-python/src/test/resources/types/example-types.yml
 types:
   definitions:
     default-package: com.palantir.product

diff --git a/conjure-python-core/src/test/resources/types/expected/package_name/product/__init__.py b/conjure-python-core/src/test/resources/types/expected/package_name/product/__init__.py
@@ -469,7 +469,7 @@ def item(self):
 
 class OptionsUnion(ConjureUnionType):
 
-    _options = None # type: str
+    _options_ = None # type: str
 
     @builtins.classmethod
     def _options(cls):
@@ -727,7 +727,7 @@ class UnionTypeExample(ConjureUnionType):
     _set = None # type: List[str]
     _this_field_is_an_integer = None # type: int
     _also_an_integer = None # type: int
-    _if = None # type: int
+    _if_ = None # type: int
     _new = None # type: int
     _interface = None # type: int