Remove auto value #159

RicoYao · 2019-02-15T00:12:18Z

Removes the use of AutoValue

Implement Builder pattern on our own.

Add ability to merge models.

rahul-malik · 2019-02-22T07:23:16Z

PR is looking good, mind running “make integration_test” to update the java fixtures in the repo and updating your PR?

rahul-malik · 2019-02-22T07:27:31Z

Sources/Core/JavaModelRenderer.swift

+    func renderBuilderMerge() -> JavaIR.Method {
+        let body = (self.transitiveProperties.map { param, _ in
+            "if (model.get" + param.snakeCaseToCapitalizedPropertyName() + "IsSet()) {\n" +
+                "    this." + param.snakeCaseToPropertyName() + " = model." + param.snakeCaseToPropertyName() + ";\n" +


We generally don’t indent generated code like this throughout the repository. Ok general we rely on the “—>” operator to indent a scoped region.

The reason is because we insert tabs and then post process them out to whatever the correct tabwidth is per language

Done (I used --> in the ifBLock method that you asked me to add in your other comment). I also switched over the other if-statements in this PR to use it.

rahul-malik · 2019-02-22T07:28:11Z

Sources/Core/JavaModelRenderer.swift

+
+    func renderBuilderMerge() -> JavaIR.Method {
+        let body = (self.transitiveProperties.map { param, _ in
+            "if (model.get" + param.snakeCaseToCapitalizedPropertyName() + "IsSet()) {\n" +


Can we add a JavaIR.if method and use it here?

Done. I had to call it ifBlock because just "if" wasn't allowed.

rahul-malik · 2019-02-22T07:36:19Z

Sources/Core/JavaModelRenderer.swift

-            "return new AutoValue_\(className).GsonTypeAdapter(gson);"
+    func renderModelMergeWith() -> JavaIR.Method {
+        return JavaIR.method([.public], self.className + " mergeWith(" + self.className + " model)") {[
+            self.className + ".Builder builder = this.toBuilder();",


FYI you can use string interpolation in swift to inline simple strings line this
“(self.className).Builder ...”

I can switch the whole thing over if you feel strongly, but I really found it hard to read/code particularly because of the need to include a "\". I spoke to some in-house iOS folks who seemed to feel that concatenation through + was fine.

I think as long as we're not executing functions inside \(), for simple strings this is a bit easier to read but that's just my opinion. I'd prefer that they were using string interpolation for simple strings

rahul-malik · 2019-02-22T07:38:20Z

Sources/Core/JavaModelRenderer.swift

-        return JavaIR.method([.public, .static], "TypeAdapter<\(className)> jsonAdapter(Gson gson)") {[
-            "return new AutoValue_\(className).GsonTypeAdapter(gson);"
+    func renderModelMergeWith() -> JavaIR.Method {
+        return JavaIR.method([.public], self.className + " mergeWith(" + self.className + " model)") {[


String interpolation for the method name as well

Same response as before.

RicoYao · 2019-03-06T00:14:14Z

"mind running “make integration_test” to update the java fixtures in the repo and updating your PR?"
-- done

- Remove autovalue annotations/imports - Add builder class

Use --> instead of custom indentation Create JavaIR.ifBlock

…ed-in model wins

rahul-malik · 2019-03-06T03:30:35Z

Will review soon, also I filed this to track actually building the java code during integration tests
#169

Examples/Java/Sources/Board.java

rahul-malik · 2019-03-06T20:38:07Z

Examples/Java/Sources/Board.java

+            return false;
+        }
+        Board that = (Board) o;
+        return Objects.equals(this.identifier, that.identifier) &&


For later, in ObjC we sorted the equality comparisons to compare primitive properties first since that was a bit faster

I'll file a ticket for this as well

rahul-malik · 2019-03-06T20:39:57Z

Examples/Java/Sources/Board.java

+        @SerializedName("name") private @Nullable String name;
+        @SerializedName("url") private @Nullable String url;
+         private int _bits = 0;
+        private Builder() {


Do we need an empty builder init here?

It's not a hard requirement right now (creating a Model from scratch), but I'm about to introduce a custom GSON TypeAdapter that goes through the Builder (so that we properly set _bits), and it will use this.

Is there anything proprietary about this adapter? If it’s pretty general it would be useful to include here for others to utilize.

@RicoYao - We can resolve this in a follow-up. I'll file an issue

@rahul-malik Sorry I should have been more clear, the plan is to submit the TypeAdapter into the master branch (working on the pull request).

Sources/Core/JavaIR.swift

Examples/Java/Sources/Image.java

rahul-malik · 2019-03-06T20:50:45Z

@RicoYao - Please take a look at the comments and let me know what you think

RicoYao · 2019-03-07T17:16:10Z

@rahul-malik Replied in-line.

rahul-malik · 2019-03-07T20:13:38Z

Examples/Java/Sources/Board.java

+        @SerializedName("name") private @Nullable String name;
+        @SerializedName("url") private @Nullable String url;
+         private int _bits = 0;
+        private Builder() {


@RicoYao - We can resolve this in a follow-up. I'll file an issue

rahul-malik · 2019-03-07T20:14:29Z

Examples/Java/Sources/Board.java

+            return false;
+        }
+        Board that = (Board) o;
+        return Objects.equals(this.identifier, that.identifier) &&


I'll file a ticket for this as well

rahul-malik · 2019-03-07T20:19:10Z

@RicoYao - I'll merge this now, can you get yourself added to the Pinterest Github Organization so I can assign you tasks? The #opensource room should be able to help :)

RicoYao · 2019-03-07T21:04:05Z

@rahul-malik Thanks, I didn't realize you would actually merge it, but it's helpful so I can work on smaller follow-up pull requests. Please check in before producing a release though, as I'd want to run some extra tests.

rahul-malik · 2019-03-07T22:59:43Z

@RicoYao Yeah I think it was far enough along where it should be merged, I wanted to merge it so I could look into actually building the code. We'll work together before drafting a release candidate which will involve actually having these models building as part of CI

That said, I did notice an issue when I tried to compile some of the code in the private initializers #174

RicoYao · 2019-03-08T00:22:54Z

@rahul-malik I think I broke it in my very last commit when I was trying to fix the tabbing. I normally test it in my Java IDE, but forgot on the last one. Will fix. Thanks!

RicoYao · 2019-03-08T00:32:11Z

@rahul-malik Also, just FYI, I'd been loading a Plank-generated model into diff D360909 for a compilation check.

rahul-malik reviewed Feb 22, 2019

View reviewed changes

RicoYao added 13 commits March 5, 2019 17:15

First pass at removing AutoValue

c703c13

- Remove autovalue annotations/imports - Add builder class

Remote autotype annotation, add missing semi-colons

5038df6

Add SerializedName annotation to Builder properties

c7601ea

add isSet checkers and property-getters

37d5574

missing "this" prefix

fff92fe

Fix swapped method names equals/hashcode

d2d31eb

added merge

d7be793

fix mergeWith, reorder methods for cleanup

a44baf7

Add builder getters

cabedeb

make lint

3db85c6

Generage Java examples

51c2011

Use --> instead of custom indentation Create JavaIR.ifBlock

Rename method mergeWith to mergeFrom to make it clearer that the pass…

4e72815

…ed-in model wins

Rebase and do make format

9d8f956

RicoYao force-pushed the remove_auto_value branch from b793e4e to 9d8f956 Compare March 6, 2019 01:29

rahul-malik reviewed Mar 6, 2019

View reviewed changes

Examples/Java/Sources/Board.java Show resolved Hide resolved

rahul-malik reviewed Mar 6, 2019

View reviewed changes

Clean up some tabbing and spaces. Address feedback.

fed111d

rahul-malik approved these changes Mar 7, 2019

View reviewed changes

rahul-malik merged commit 7d17623 into pinterest:master Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove auto value #159

Remove auto value #159

RicoYao commented Feb 15, 2019

rahul-malik commented Feb 22, 2019

rahul-malik Feb 22, 2019

RicoYao Mar 6, 2019

rahul-malik Feb 22, 2019

RicoYao Mar 6, 2019

rahul-malik Feb 22, 2019

RicoYao Mar 6, 2019 •

edited

Loading

rahul-malik Mar 6, 2019

rahul-malik Feb 22, 2019

RicoYao Mar 6, 2019

RicoYao commented Mar 6, 2019

rahul-malik commented Mar 6, 2019 •

edited

Loading

rahul-malik Mar 6, 2019

rahul-malik Mar 7, 2019

rahul-malik Mar 6, 2019

RicoYao Mar 6, 2019

rahul-malik Mar 7, 2019

rahul-malik Mar 7, 2019

RicoYao Mar 7, 2019

rahul-malik commented Mar 6, 2019

RicoYao commented Mar 7, 2019

rahul-malik Mar 7, 2019

rahul-malik Mar 7, 2019

rahul-malik commented Mar 7, 2019

RicoYao commented Mar 7, 2019

rahul-malik commented Mar 7, 2019

RicoYao commented Mar 8, 2019

RicoYao commented Mar 8, 2019

Remove auto value #159

Remove auto value #159

Conversation

RicoYao commented Feb 15, 2019

rahul-malik commented Feb 22, 2019

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

RicoYao Mar 6, 2019 • 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

RicoYao commented Mar 6, 2019

rahul-malik commented Mar 6, 2019 • 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

rahul-malik commented Mar 6, 2019

RicoYao commented Mar 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul-malik commented Mar 7, 2019

RicoYao commented Mar 7, 2019

rahul-malik commented Mar 7, 2019

RicoYao commented Mar 8, 2019

RicoYao commented Mar 8, 2019

RicoYao Mar 6, 2019 •

edited

Loading

rahul-malik commented Mar 6, 2019 •

edited

Loading