inferred / FreeBuilder

Automatic generation of the Builder pattern for Java
Apache License 2.0
838 stars 101 forks source link

Custom functional interfaces #320

Closed alicederyn closed 5 years ago

alicederyn commented 5 years ago

Allow users to change the functional interface used in map/mutate methods with an override:

@FreeBuilder
interface MyType {
    long value();
    static class Builder extends MyType_Builder {
        @Override public Builder mapValue(LongUnaryOperator mapper) {
            return super.mapValue(mapper);
        }
    }
}

This enables a temporary work-around for #287, but also gives us a way to change the default in v2 while allowing users to keep their types binary compatible if they need to.

Another option would be to allow users to specify functional interfaces with an annotation at the class, method or package level.

The new FunctionalType class encapsulates detecting functional interfaces accepted by methods, including checking their type signatures match, and finding the right method to invoke on them.

masterav10 commented 5 years ago

I will review this either today or tomorrow.

masterav10 commented 5 years ago

I am in favor of supporting some form of "global override". I have some data objects that have 10+ primitive values, which makes the overriding substantially more cumbersome. Although it's possible that I won't need to adjust the map for every one of them.

Some additional candidates:

  1. An annotation at various levels (as you described). Downside is code modification (to a lesser extent than the current implementation).
  2. Two separate jars. Downside is more code + management on FreeBuilder.
  3. System-level property passed to the processor, if this is possible. Makes configuration potentially more complicated, as FreeBuilder is more or less configuration free right now.

I can live with the current implementation, however.

alicederyn commented 5 years ago

I didn't think you needed binary back-compat for your jars?

masterav10 commented 5 years ago

I do not, and would prefer if primitive interfaces were the default. Unless I missed something, UnaryOperator and Consumer are still the defaults for primitives, which would mean I would need to override each map method to benefit.

To be precise, in v2 if I defined the class as shown above, I would expect the signature in MyType_Builder to look like this: public Builder mapValue(LongUnaryOperator mapper)

Right now, we get: public Builder mapValue(UnaryOperator<Long> mapper)

alicederyn commented 5 years ago

The v2 branch isn't finished yet :)

masterav10 commented 5 years ago

Indeed; after looking at the commits I realize this pre-dates v2. So yeah, I think this a good compromise for now. Also clearly need to work on that reading comprehension as you literally said this.

alicederyn commented 5 years ago

@masterav10 Could you give v1.15.0 a whirl and see if it's working properly? Tests are passing but nothing like kicking the tyres to make sure there's nothing lurking.

masterav10 commented 5 years ago

On it.

==Update==

Verified on my production code. I was able to replace UnaryOperator with LongUnaryOperator. Also noticed that the behavior of nested FreeBuilder objects does not use Builders by default internally anymore, which should reduce my memory footprint for enum's that implement a FreeBuilder interface. Sweetness.

In eclipse, the interface appears to be 'sticky'. To be precise, when I override the method and change the interface, I have a compilation error until I do an Eclipse 'clean'. It is at this point that the Builder gets the proper signature. Even if I add or remove fields (whose methods are properly generated) and change the overridden method, the Builder continues to have the last signature until I do the clean. So far that's the only issue I've encountered. I'm using 2.1.0 for gradle-processors.