Allow to annotate private fields based on their getters and setters #144

geralt-encore · 2017-03-05T13:46:01Z

#133
Well, it took much more that I thought initially. Now it is possible to apply @EpoxyAttribute directly on Kotlin properties without @JvmField.

There is quite some code duplication with these changes, but I didn't find a better solution. Please, feel free to suggest any improvements, since I am not really happy with it myself.

Also, there is a weird issue that a couple of my tests are failing. I am checking that compilation fails with a specific error, but it contains only final compilation error in generated code without any mentions of the initial error. And I am pretty sure that I logged it correctly since the code I am checking with these tests is pretty trivial. So it would be nice if you could help me to deal with it.

elihart · 2017-03-05T18:14:36Z

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

@@ -142,6 +155,43 @@ private boolean isFieldPackagePrivate(Element attribute) {
  }

  /**
+   * Checks if the given private field has getter and setter for access to it
+   */
+  private void findGetterAndSetterForPrivateField(ErrorLogger errorLogger) {


To simplify, what if we assumed/enforced that the getter/setter have the same name as the field. Our generated getter/setter will already share that name so it seems like it would be a good thing for consistency anyway. Does this make sense in kotlin?

I previously made a ProcessorUtilsgetMethodOnClass method that you may be able to reuse to simplify this, although you may want to add some modifier checking into that method (to check !static, etc).

The thing is that we can't enforce it since Kotlin generate automatically getters and setters for properties with this get/set naming pattern.

ah makes sense. oh well, no big deal

elihart · 2017-03-05T18:16:46Z

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

+                + "       // value by wrapping it with this anonymous click listener\n"
+                + "       return $L.hashCode();\n"
+                + "    }\n"
+                + "  });\n", attribute.setter(), viewClickListenerType,


is using the setter vs assigning the field directly the only difference? Maybe we can keep the main code block the same, but assign it to a local variable.

Then in an if/else we can either call the super setter or set the super field. That way we could reuse most of the code

Yep, just wasn't sure how to proceed with it. I managed to solve it. Will include into next commit.

elihart · 2017-03-05T18:24:19Z

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

@@ -623,6 +656,41 @@ private void addEqualsLineForType(Builder builder, boolean useObjectHashCode, Ty
    }
  }

+  private void addEqualsLineForTypeUsingGetter(Builder builder, boolean useObjectHashCode,


To avoid duplicating hash/equals methods, what if we made a little method like attributeGetterCode and attributeSetterCode, which would so something like

if(isPrivateAttribute){ return attribute.name + "()"; // Assuming we enforce setter name has to equal field name } else { return attribute.name; } Setter might be trickier but maybe possible?

This could also be a method on the attribute info -> attributeInfo.getAccessorCode()

I think this approach could clean up the code in all the usages above too

I've made it slightly different. addEqualsLineForTypeUsingGetter is not needed anymore. Same for hash code generation.

elihart

Awesome, thanks! I see what you mean about the code duplication, I think we can figure out some ways to improve it, but this seems like a great start. I gave it a brief look but will try to spend more time on it soon.

elihart · 2017-03-05T20:09:52Z

Not having the right compilation error is weird. Probably related to my changes to group all errors at the end. I'll try running these tests locally and see if I can debug.

I was thinking recently that we could make a script that updates failing processor tests with the actual output.

The script could take the filepath of the report, like repos/epoxy/epoxy-processortest/build/reports/tests/debug/index.html, find tests that failed because of processed code mismatch (it says something like "actual source" and "expected source", and it could update the file of the expected source to contain the actual code.

This would be very helpful when we change a small thing in the processor that affects all processor tests. We know the actual code is right, but would have to manually update the test code. I know you've had to do that in the past :P, and I 've been wasting a lot of time on it recently with my refactor.

elihart · 2017-03-05T20:38:56Z

Fyi #145 will cause a small merge conflict, but it is just adding a view param to the model click listener in the generated code in tests.

If you prefer I can wait to merge it until after this PR is merged

geralt-encore · 2017-03-05T21:52:39Z

Go ahead! I'll resolve a merge conflict. It's not a big deal.

elihart · 2017-03-05T21:58:51Z

Thanks. I'm gonna try my hand at a ruby script to automatically update processor tests, so maybe don't try changing your tests for now (if you change processor code)

elihart · 2017-03-05T23:11:28Z

I think I have the script working (albeit it's ugly :P)

require 'rubygems'
require 'nokogiri'

def updateTestClass(test_class_result)
  page = Nokogiri::HTML(open(test_class_result))

  page.css('pre').each do |preBlock|
    if preBlock.include? "Source declared the same top-level types of an expected source"
      next
    end

    expected_source_file_path = /Expected file: <([^>]*)>/m.match(preBlock).captures[0]

    expected_source_file_path.sub! "build/intermediates/classes/test/debug", "src/test/resources"

    actual_source_match = /Actual Source:[\s]*=*[\s]*(package.*?})[\s]*at com\.google/m.match(preBlock)
    actual_source = actual_source_match.captures[0]

    File.open(expected_source_file_path, "w") do |f|
      f.write actual_source
    end
  end
end

module_name = ARGV[0]
classes_directory_name = module_name + "/build/reports/tests/debug/classes"

Dir.glob(classes_directory_name + "/*.html") do |test_class_result|
  updateTestClass(test_class_result)
end

you would first run ./gradlew test, and if tests fail you can run this script to replace the failing expected source with the actual source.

You just provide the module name of the tests, like:
ruby TestUpdater.rb epoxy-processortest

I've only tested briefly. I need to go through and clean it up, but if you want to try it for you own usage to save some time, go for it!

I'm not great with ruby, so if you see opportunity for improvement, please go for it. I'll put this up as a PR later when I have time. It also assumes it is being run from the root directory of the repo

elihart · 2017-03-06T05:10:39Z

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

+        ExecutableElement method = (ExecutableElement) element;
+        String methodName = method.getSimpleName().toString();
+        // check if it is a valid getter
+        if (methodName.equalsIgnoreCase(String.format("get%s", name))


should this not be equalsIgnoreCase since with a different case it would be a different method?

You are right, it was just the easiest approach. I'll handle it properly. Thanks for pointing out!

elihart · 2017-03-06T05:26:27Z

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

+        String methodName = method.getSimpleName().toString();
+        // check if it is a valid getter
+        if (methodName.equalsIgnoreCase(String.format("get%s", name))
+            || methodName.equalsIgnoreCase(String.format("is%s", name))


ah your tests are failing because your order of operations for the logic here is messed up. You need ( ) around your || operation

(methodName.equalsIgnoreCase(String.format("get%s", name)) || methodName.equalsIgnoreCase(String.format("is%s", name)))

Tricky!

Btw I recently learned how to debug annotation processing and it is very helpful. I used it to figure this out https://blog.xmartlabs.com/2016/03/28/Debugging-an-Annotator-Processor-in-your-project/

wow, thanks! That makes sense now. Yeah, I've seen this article, but I was pretty sure that the code is alright and the issue was in errors handling. Shame on me :P

elihart · 2017-03-06T05:29:02Z

epoxy-processortest/src/test/java/com/airbnb/epoxy/ModelProcessorTest.java

+  }
+
+  @Test
+  public void modelWithViewPrivateClickListener() {


Thanks for adding so many tests!

No problem, I am jus trying to cover every possible case. This is the purpose of tests after all :)

# Conflicts: # epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.java

elihart

Awesome, thanks!

I'll be sure to highlight this in the 2.0 release :)

geralt-encore · 2017-03-06T19:41:19Z

Glad that we were able to deal with it so fast :)

jbmlaird · 2017-05-23T10:59:54Z

Can you give me an example of how this is meant to work @geralt-encore ?

Using version 2.1.0 gives me the error mentioned in this thread with my code:

@EpoxyModelClass(layout = R.layout.model_text)
abstract class TextModel(@EpoxyAttribute val text: String?) : EpoxyModel<TextView>() {
    override fun bind(view: TextView?) {
        super.bind(view)
        view?.text = text
    }
}

geralt-encore · 2017-05-23T11:15:47Z

@jbmlaird text has to be var instead of val since in generated code we need access to both getter and setter and in val case only getter exists.

Allow to annotate private fields based on their getters and setters

72eb177

elihart reviewed Mar 5, 2017

View reviewed changes

elihart reviewed Mar 6, 2017

View reviewed changes

geralt-encore added 6 commits March 6, 2017 11:12

Merge remote-tracking branch 'upstream/2.0' into kotlin

eb3eea6

# Conflicts: # epoxy-processor/src/main/java/com/airbnb/epoxy/GeneratedModelWriter.java

Fix failing tests

6445636

Fix PR comments

8e3093d

Fix checkstyle

28d1f1a

Use equals instead of equalsIgnoreCase

7e7c278

Remove unused import

e29e4d0

elihart approved these changes Mar 6, 2017

View reviewed changes

elihart merged commit d11379b into airbnb:2.0 Mar 6, 2017

geralt-encore deleted the kotlin branch March 7, 2017 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to annotate private fields based on their getters and setters #144

Allow to annotate private fields based on their getters and setters #144

geralt-encore commented Mar 5, 2017 •

edited

Loading

elihart Mar 5, 2017

geralt-encore Mar 5, 2017

elihart Mar 6, 2017

elihart Mar 5, 2017

geralt-encore Mar 6, 2017

elihart Mar 5, 2017

elihart Mar 5, 2017

geralt-encore Mar 6, 2017 •

edited

Loading

elihart left a comment

elihart commented Mar 5, 2017

elihart commented Mar 5, 2017

geralt-encore commented Mar 5, 2017

elihart commented Mar 5, 2017

elihart commented Mar 5, 2017 •

edited

Loading

elihart Mar 6, 2017

geralt-encore Mar 6, 2017

elihart Mar 6, 2017

geralt-encore Mar 6, 2017

elihart Mar 6, 2017

geralt-encore Mar 6, 2017

elihart left a comment

geralt-encore commented Mar 6, 2017

jbmlaird commented May 23, 2017

geralt-encore commented May 23, 2017

Allow to annotate private fields based on their getters and setters #144

Allow to annotate private fields based on their getters and setters #144

Conversation

geralt-encore commented Mar 5, 2017 • 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

geralt-encore Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

elihart left a comment

Choose a reason for hiding this comment

elihart commented Mar 5, 2017

elihart commented Mar 5, 2017

geralt-encore commented Mar 5, 2017

elihart commented Mar 5, 2017

elihart commented Mar 5, 2017 • 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

elihart left a comment

Choose a reason for hiding this comment

geralt-encore commented Mar 6, 2017

jbmlaird commented May 23, 2017

geralt-encore commented May 23, 2017

geralt-encore commented Mar 5, 2017 •

edited

Loading

geralt-encore Mar 6, 2017 •

edited

Loading

elihart commented Mar 5, 2017 •

edited

Loading