Generate model factory methods #450

ngsilverman · 2018-06-15T22:15:10Z

Creates a new module, epoxy-modelfactory
- Houses the new ModelProperties interface
- Depends on Paris to enable ModelProperties to set styles
- Contains the relevant tests
  - I tried adding a dependency on epoxy-processortest to reuse ProcessorTestUtils but that caused mysterious Lint issues so I copied a single method from it over instead.
If this new module is present, for all models that have an empty constructor, no property group, and at least one property of a supported type, a static from(ModelProperties properties) method is generated.

  public static MyViewModel_ from(ModelProperties properties) {
    MyViewModel_ model = new MyViewModel_();
    model.id(properties.getString("id"));
    // This is repeated for each attribute of a supported type
    if (properties.has("title")) {
      model.title(properties.getString("title"));
    }
    ...
    // If it's styleable
    Style style = properties.getStyle();
    if (style != null) {
      model.style(style);
    }
    return model;
  }

TODO

Fix Lint errors
Is the new module's package name setup correct?

(I rebased and reformatted GeneratedModelWriter but nothing changed. Maybe it hadn't been properly formatted in the first place?)

ngsilverman · 2018-06-15T22:16:17Z

epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.kt

@@ -998,7 +1089,7 @@ internal class GeneratedModelWriter(
            if (attr is ViewAttributeInfo && attr.generateStringOverloads) {
                methods.addAll(StringOverloadWriter(modelInfo, attr, configManager).buildMethods())
            } else {
-                if (attr.isViewClickListener) {
+                if (attr.isViewClickListener || attr.isViewLongClickListener) {


^ this is a change (though not a functional one, I modified isViewClickListener so I could use it elsewhere).

ngsilverman · 2018-06-15T22:17:16Z

epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.kt

@@ -1295,6 +1412,106 @@ internal class GeneratedModelWriter(
        return method
    }

+    /**


And here's the main addition besides tests.

ngsilverman · 2018-06-15T22:18:03Z

epoxy-processortest/build.gradle

+
+    javaCompileOptions {
+      annotationProcessorOptions {
+        includeCompileClasspath false


This makes it so Gradle doesn't complain that there are undeclared annotation processors in the classpath.

leave a code comment about it?

That seems a bit redundant, the config itself is pretty explicit: don't include annotation processors that are in the compile classpath, which this module does because it's testing them with the google testing compile lib.

well you still left a github comment explaining it, so it isn't too explicit ;)

Only because it doesn't seem directly related to this PR. 😛

elihart · 2018-06-15T23:21:24Z

epoxy-modelfactory/src/main/java/com/airbnb/epoxy/ModelProperties.java

+  double getDouble(@NonNull String propertyName);
+
+  @NonNull
+  Drawable getDrawable(@NonNull String propertyName);


could support getDrawableRes by looking for @DrawableRes annotation

elihart · 2018-06-15T23:21:37Z

epoxy-modelfactory/src/main/java/com/airbnb/epoxy/ModelProperties.java

+import java.util.List;
+
+public interface ModelProperties {
+


dedicated getId() method?

elihart · 2018-06-15T23:21:49Z

epoxy-modelfactory/src/main/java/com/airbnb/epoxy/ModelProperties.java

+
+import java.util.List;
+
+public interface ModelProperties {


we can use kotlin for this module

elihart · 2018-06-15T23:27:49Z

epoxy-modelfactory/src/test/java/com/airbnb/epoxy/ProcessorTestUtils.kt

+
+internal object ProcessorTestUtils {
+    @JvmOverloads
+    @JvmStatic


don't need the jvm helpers

elihart · 2018-06-15T23:51:17Z

epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.kt

+        }
+
+        val supportedAttributeInfo = modelInfo.attributeInfo.filter { attributeInfo ->
+            listOf(


might be easier to read to have this list declared outside the loop

elihart · 2018-06-15T23:53:18Z

epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.kt

+                    attributeInfo.isDrawable -> "getDrawable"
+                    attributeInfo.isInt -> "getInt"
+                    attributeInfo.isViewClickListener -> "getOnClickListener"
+                    else -> throw IllegalStateException()


Put a message in the exception

elihart · 2018-06-15T23:53:43Z

epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.kt

+        }
+
+        // Groups aren't supported because we wouldn't know which type to choose from
+        if (modelInfo.attributeGroups.any { it.attributes.size > 1 }) {


check this before supported attributes?

elihart · 2018-06-15T23:53:58Z

epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.kt

+        }
+
+        // If none of the properties are of a supported type the method isn't generated
+        if (supportedAttributeInfo.isEmpty()) {


should it be an error if any attributes are not supported?

elihart · 2018-06-15T23:55:30Z

epoxy-processortest/build.gradle

+
+    javaCompileOptions {
+      annotationProcessorOptions {
+        includeCompileClasspath false


leave a code comment about it?

…annotations

…ttributes of a supported type

ngsilverman · 2018-06-16T01:03:14Z

@elihart addressed all the feedback. 😄

elihart

Great! ship it 🚀

elihart · 2018-06-16T12:55:33Z

epoxy-modelfactory/src/main/java/com/airbnb/epoxy/ModelProperties.kt

+
+interface ModelProperties {
+
+    fun getId(): String


why fun over val?

With everything else being a function it seemed inconsistent/out of place. 🤷‍♂️

elihart · 2018-06-16T12:58:12Z

epoxy-processortest/build.gradle

+
+    javaCompileOptions {
+      annotationProcessorOptions {
+        includeCompileClasspath false


well you still left a github comment explaining it, so it isn't too explicit ;)

…ives issue

elihart

@ngsilverman great, updates look nice. thanks!

elihart · 2018-06-26T00:51:33Z

epoxy-processor/src/main/java/com/airbnb/epoxy/AttributeInfo.java

+  boolean isEpoxyModelList() {
+    return Utils.isType(
+        getTypeMirror(),
+        "java.util.List<? extends com.airbnb.epoxy.EpoxyModel<?>>"


what about kotlin support?

@elihart after doing some testing I just realized that we don't need to check for Kotlin types at all (for lists, strings or charsequences alike). Guess they get converted to Java types for interop.

elihart · 2018-06-26T00:52:50Z

epoxy-processor/src/main/java/com/airbnb/epoxy/AttributeInfo.java

+  }
+
+  boolean isStringList() {
+    return Utils.isType(getTypeMirror(), "java.util.List<java.lang.String>")


maybe a good way to DRY up the two list checks is to have a isList(type) method that checks for java/kotlin lists and also checks the generic type as a parameter

elihart · 2018-06-26T00:53:19Z

epoxy-processor/src/main/java/com/airbnb/epoxy/AttributeInfo.java

+  boolean isEpoxyModelList() {
+    return Utils.isType(
+        getTypeMirror(),
+        "java.util.List<? extends com.airbnb.epoxy.EpoxyModel<?>>"


would be nice to use a ClassName constant or something, not sure if that is possible with the wildcard

elihart · 2018-06-26T00:55:04Z

epoxy-processor/src/main/java/com/airbnb/epoxy/Utils.java

+        || isStringType(type);
+  }
+
+  static boolean isStringAttributeDataType(TypeMirror type) {


better to have a generic isType(TypeMirror , ClassName) method?

elihart · 2018-06-26T00:57:36Z

epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.kt

+    ) {
+        // The epoxy-modelfactory module must be present to enable this functionality
+        val hasModelFactoryDependency =
+            getTypeMirror(EPOXY_MODEL_PROPERTIES.reflectionName(), elements) != null


opportunity for an elements.isTypeLoaded(ClassName) extension

elihart · 2018-06-26T00:57:59Z

epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.kt

+
+        // Models that don't have an empty constructor are not supported because there would be no
+        // clear way to create new instances
+        val hasEmptyConstructor = modelInfo.constructors.isEmpty()


could this be a method on the ModelInfo class?

ngsilverman · 2018-06-27T20:34:33Z

@elihart Added a Kotlin view test and followed your suggestions. Let me know if this is good to go. Do you want me to handle the release?

elihart · 2018-06-27T21:30:23Z

@ngsilverman yeah, seems good to go. if you can do the release that'd be great

ngsilverman added the enhancement label Jun 15, 2018

ngsilverman requested a review from elihart June 15, 2018 22:15

ngsilverman commented Jun 15, 2018

View reviewed changes

ngsilverman force-pushed the nsilverman-modelfactory branch from 6e0e8fb to f101814 Compare June 15, 2018 22:39

Generate model factory methods

d4ee671

ngsilverman force-pushed the nsilverman-modelfactory branch from f101814 to d4ee671 Compare June 15, 2018 22:41

ngsilverman added 2 commits June 15, 2018 18:34

[modelfactory] Add explicit getId() method to ModelProperties

5f70999

[modelfactory] Convert ModelProperties to Kotlin

003dfd6

elihart reviewed Jun 15, 2018

View reviewed changes

ngsilverman added 5 commits June 15, 2018 19:26

[modelfactory] Replace getDrawable with getDrawableRes

3f14601

[modelfactory] ProcessorTestUtils: remove unnecessary Kotlin interop …

2cfea41

…annotations

[modelfactory] Support attribute groups if they don't have multiple a…

a44ad6a

…ttributes of a supported type

[modelfactory] Add error message

4f63e41

[modelfactory] Misc

e4b098a

elihart approved these changes Jun 16, 2018

View reviewed changes

ngsilverman added 4 commits June 20, 2018 11:04

[modelfactory] Convert ModelProperties back to Java to fix uploadArch…

26602e8

…ives issue

[modelfactory] Fix CharSequence/String type checks

ee5272e

[modelfactory] Add support for lists of Epoxy models

71d1078

[modelfactory] Fix support for "legacy" models (non @ModelView)

d8917ec

elihart approved these changes Jun 26, 2018

View reviewed changes

ngsilverman added 2 commits June 27, 2018 13:33

[modelfactory] Add Kotlin view test

bc200be

[modelfactory] Address PR feedback

87ccc38

ngsilverman merged commit ed916a0 into master Jun 27, 2018

ngsilverman deleted the nsilverman-modelfactory branch June 27, 2018 22:42

ngsilverman mentioned this pull request Jun 27, 2018

Prepare for 2.14.0 release #460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate model factory methods #450

Generate model factory methods #450

ngsilverman commented Jun 15, 2018 •

edited

Loading

ngsilverman Jun 15, 2018

ngsilverman Jun 15, 2018

ngsilverman Jun 15, 2018

elihart Jun 15, 2018

ngsilverman Jun 16, 2018 •

edited

Loading

elihart Jun 16, 2018

ngsilverman Jun 18, 2018

elihart Jun 15, 2018

elihart Jun 15, 2018

elihart Jun 15, 2018

elihart Jun 15, 2018

elihart Jun 15, 2018

elihart Jun 15, 2018

elihart Jun 15, 2018

elihart Jun 15, 2018

elihart Jun 15, 2018

ngsilverman commented Jun 16, 2018 •

edited

Loading

elihart left a comment

elihart Jun 16, 2018

ngsilverman Jun 18, 2018

elihart Jun 16, 2018

elihart left a comment

elihart Jun 26, 2018

ngsilverman Jun 27, 2018

elihart Jun 26, 2018

elihart Jun 26, 2018

elihart Jun 26, 2018

elihart Jun 26, 2018

elihart Jun 26, 2018

ngsilverman commented Jun 27, 2018

elihart commented Jun 27, 2018

Generate model factory methods #450

Generate model factory methods #450

Conversation

ngsilverman commented Jun 15, 2018 • edited Loading

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

ngsilverman Jun 16, 2018 • edited Loading

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

ngsilverman commented Jun 16, 2018 • edited Loading

elihart left a 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

elihart left a 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

ngsilverman commented Jun 27, 2018

elihart commented Jun 27, 2018

ngsilverman commented Jun 15, 2018 •

edited

Loading

ngsilverman Jun 16, 2018 •

edited

Loading

ngsilverman commented Jun 16, 2018 •

edited

Loading