mergeFrom() on supertypes and their builders #220

ph4r05 · 2016-12-08T16:43:17Z

Hi everyone,
here is my implementation of the mergeFrom() for super types. It basically addresses #160. I am already using in in my project.

It supports mergeFrom() super types and their builders (if the super type is correctly annotated with the FreeBuilder annotation).

I've added one minor test to the analysator. I can add more but at first it would be great to know if my implementation is acceptable.

The #176 can be solved with this as well (metadata now have record about supertypes with builders).

Cheers

- merge from super types builders is allowed only if the type and super type are in the same package due to Enum Property visibility and non-default values.

googlebot · 2016-12-08T16:43:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2016-12-08T16:43:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

ph4r05 · 2016-12-08T16:46:23Z

I signed it!

googlebot · 2016-12-08T16:46:26Z

CLAs look good, thanks!

googlebot · 2016-12-08T16:46:26Z

CLAs look good, thanks!

alicederyn

Hi @ph4r05, and thanks for contributing! I've attached my high-level feedback: there are a number of critical issues you'll need to address before we get to work on cleanup/testing. Feel free to chat me on https://gitter.im/inferred-freebuilder/Lobby !

alicederyn · 2016-12-09T17:09:57Z

src/test/java/org/inferred/freebuilder/processor/GenericTypeSourceTest.java

+        "   * Returns true if the required property corresponding to",
+        "   * {@link Person#getName()} is set.",
+        "   */",
+        "  public boolean isPropertyNameSet() {",


As per discussion on issue #41, I'm not happy to have these methods added: they can't be removed when a default is set; and they make customization harder. Use a try/catch loop to discard the IllegalStateException thrown by getName().

alicederyn · 2016-12-09T17:11:12Z

src/main/java/org/inferred/freebuilder/processor/Metadata.java

+  public abstract ImmutableSet<ParameterizedType> getSuperBuilderTypes();
+
+  /** Returns super type properties */
+  public abstract ImmutableMap<ParameterizedType, ImmutableList<Property>> getSuperTypeProperties();


I'd prefer this map stored a new value type containing a set of properties and a boolean indicating whether there is a builder or not.

Also, generic builders and generic supertypes will be problematic. For generic builders, you'll have to be very careful you're putting the right type parameters in the right places (which you may already be doing, but I'd need to see tests). For generic supertypes, you'll need to figure out the correct wildcard parameterisation to put in.

You'll need to factor in visibility as well; we can't accidentally generate a method accepting a package-scoped type from another package, as that's a compile error.

alicederyn · 2016-12-09T17:15:12Z

src/main/java/org/inferred/freebuilder/processor/Analyser.java

+
+      // Code builder dance
+      if (builder.isPresent()) {
+        final Metadata metadataSuperType = analyse(superType);


We've already figured out what properties are implied by what getter methods; we should just check to see which of those methods are declared abstract on this supertype, rather than regenerating everything.

Also, you can't call analyse on non-FreeBuilder-annotated types, as they shouldn't be expected to follow FreeBuilder's rules about abstract methods.

Oh, and covariant return types are going to cause issues. You can either identify those occasions where runtime type checking is going to be safe (not forgetting optionals, collections, etc. can typecheck members), or bail out entirely if the return type doesn't exactly match what you'd expect. I'd recommend the latter, especially as a first PR.

alicederyn · 2016-12-09T17:23:34Z

src/main/java/org/inferred/freebuilder/processor/Analyser.java

+      }
+
+      final Optional<AnnotationMirror> freeBuilderMirror =
+              findAnnotationMirror(superType, FreeBuilder.class);


This won't work: the FreeBuilder annotation is source-only. If your two types are not compiled together, or if your IDE decides to compile the two classes separately (e.g. you change your subtype in Eclipse), you'll be given the class definition from the compiled JAR. Check out the logic in BuildablePropertyFactory, for example. In this case, however, you'll need to check for the presence of all the getter methods too, since a user might have generated a broken builder by hand.

alicederyn · 2016-12-09T17:49:17Z

src/main/java/org/inferred/freebuilder/processor/Analyser.java

+        }
+      }
+
+      toRet.put(pType, ImmutableList.copyOf(superPropertiesRet.values()));


We don't want to generate a merge method for every single supertype, only those which declare a getter method.

alicederyn · 2017-03-07T15:58:10Z

Closing due to lack of activity. I hope you manage to get back to this sometime, @ph4r05

mergeFrom() added for super types and super type builders

aa6f182

- merge from super types builders is allowed only if the type and super type are in the same package due to Enum Property visibility and non-default values.

ph4r05 force-pushed the merge-supertypes branch from fb3afcd to 57b22dd Compare December 8, 2016 17:27

mergeFrom() super type builder works across the packages

b1c924e

ph4r05 force-pushed the merge-supertypes branch from 57b22dd to b1c924e Compare December 8, 2016 17:34

ph4r05 changed the title ~~Merge supertypes - #160~~ mergeFrom() on supertypes - #160 Dec 8, 2016

ph4r05 force-pushed the merge-supertypes branch 4 times, most recently from 1f468f0 to 4475921 Compare December 8, 2016 20:14

property factory fixes (mergeFromSuperBuilder)

f592eb8

ph4r05 force-pushed the merge-supertypes branch from 4475921 to f592eb8 Compare December 8, 2016 20:16

ph4r05 changed the title ~~mergeFrom() on supertypes - #160~~ mergeFrom() on supertypes and their builders Dec 9, 2016

alicederyn suggested changes Dec 9, 2016

View reviewed changes

alicederyn closed this Mar 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mergeFrom() on supertypes and their builders #220

mergeFrom() on supertypes and their builders #220

ph4r05 commented Dec 8, 2016 •

edited

Loading

googlebot commented Dec 8, 2016

googlebot commented Dec 8, 2016

ph4r05 commented Dec 8, 2016

googlebot commented Dec 8, 2016

googlebot commented Dec 8, 2016

alicederyn left a comment •

edited

Loading

alicederyn Dec 9, 2016

alicederyn Dec 9, 2016

alicederyn Dec 9, 2016

alicederyn Dec 9, 2016

alicederyn Dec 9, 2016

alicederyn Dec 9, 2016

alicederyn Dec 9, 2016

alicederyn Dec 9, 2016

alicederyn commented Mar 7, 2017

mergeFrom() on supertypes and their builders #220

mergeFrom() on supertypes and their builders #220

Conversation

ph4r05 commented Dec 8, 2016 • edited Loading

googlebot commented Dec 8, 2016

googlebot commented Dec 8, 2016

ph4r05 commented Dec 8, 2016

googlebot commented Dec 8, 2016

googlebot commented Dec 8, 2016

alicederyn left a comment • 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

alicederyn commented Mar 7, 2017

ph4r05 commented Dec 8, 2016 •

edited

Loading

alicederyn left a comment •

edited

Loading