Basic support for builder inheritance #132
Conversation
|
Does this handle exclusion of properties properly? If you exclude a superclass property I suspect that the coresponding with-method will not be overriden in the subclass builder. This is a problem because then the return type is that of the superclass and the method hast no effect. I can't check that right now, but I don't see a test for this scenario, so I guess you missed this.
|
|
Although it is not really important it might be good to reuse the fields in the superclass builder, but that might be too difficult. |
|
a) I seem to broken a test in my final "this little tidy up can't break anything" commit. Will have to look into that in a day or so. |
|
Finally I got into this since I am working on #133. As you stated, it would be better to have an interface generated instead of an abstract builder. I'll see if I can come up with something usable today. |
|
Well, regardless what I try, I can't solve this one. For example, I just did a checkout of your branch 'clone-fix' and modified your test scenario like this: package net.karneim.pojobuilder.processor.with.builderinheritance;
import net.karneim.pojobuilder.GeneratePojoBuilder;
@GeneratePojoBuilder(withBuilderInterface = Builder.class)
public class Fruit {
public String colour;
}
@GeneratePojoBuilder(withBaseclass = FruitBuilder.class, withBuilderInterface = Builder.class)
class Apple extends Fruit {
public String variety;
}As you can see I added This gives an compilation error: This error would be the same in PB 3.5.0. That's why it is not possible to create a working builder hierarchy. It is still not possible with this branch. |
|
You can achieve that by using an assertj style selftype generic. This would require all superclass builders to be generic and everyone who is already using PB would geht a lot of raw type warnings when updating. |
|
For the classes Fruit and Apple: @GeneratePojoBuilder(withBuilderInterface = Builder.class, generateCommonBuilderInterface=true)
public class Fruit {
public String colour;
}
@GeneratePojoBuilder(withBuilderInterface = Builder.class, generateCommonBuilderInterface=true)
class Apple extends Fruit {
public String variety;
}you could additionally generate a "common builder interface" IFruitBuilder: public interface IFruitBuilder<T extends Fruit> implements Builder<T> {
IFruitBuilder<T> withColour(String colour);
}
public class FruitBuilder implements IFruitBuilder<Fruit> { ... }
public class AppleBuilder implements IFruitBuilder<Apple> { ... }This would avoid the issue of adding generics to old builder classes which would generate a lot of raw type warnings. |
|
@Adrodoc55 I know that we could use the selftype generic style. Actually I described this solution in issue #55. I have not implemented it because of its own uglyness that you pointed out correctly: users must add genetic type arguments to all their builder usages. The idea of generating separate interfaces for each builder looks promising, although I could not create a working scenario where all PB features would work. In your example both builders just implement the IFruitBuilder interfaces. The more real life scenario would be that both implement their own specific interface. But that does issue the same compilation error I mentioned earlier in this thread. Edit: |
|
The point is, that I think the only way we have to support builder hierarchies is to go with the selftype style, and if we want to release our users from specifying generic type arguments all over their code we must provide some more code that does this automatically, e.g. a factory or builder specific subclasses that just do that. |
|
We actually could do this using interfaces if the concrete builders do not extend each other. I mean this: public interface IFruitBuilder<T extends Fruit> extends Builder<T> {
IFruitBuilder<T> withColour(String colour);
}
public interface IAppleBuilder<T extends Apple> extends IFruitBuilder<T> {
IAppleBuilder<T> withVariety(String variety);
}
public class FruitBuilder implements IFruitBuilder<Fruit> { ... }
public class AppleBuilder implements IAppleBuilder<Apple> { ... }Edit There is also a main method that shows some use case: https://github.com/mkarneim/pojobuilder-playground/blob/master/src/main/java/fruits/Main.java What do you think? Would this work? |
|
I made some small changes that take a step forwards because the "proper fix" eludes us. We all know this by now :) Can my "baby steps" changes get into the build while we think further as it could take (more) years! My thoughts (and I thought a lot on this) have been: Q Interaction with BuilderInterface is ugly but certainly possible. There are several alternatives that would need mocking up to discover which gives the cleanest solution. |
|
I think the best way to discuss different approaches is by showing complete examples like the one I created with https://github.com/mkarneim/pojobuilder-playground. I like the interface variant for it's simplicity and the fact that the user is not bothered with generic self types. |
|
Your playground example has the same issues, specifically Extending from this PR, my proposals for next steps are:
Again though - this is conversation about the big picture. I'm really keen to contribute and make inheritable builders happen but could you review this PR as what it is not everything it could be.
|
|
@drekbour The directive |
|
PB still doesn't support this! There is no logical reason to reject "abstract Builders" because you prefer interfaces. In our current code, that "builder interface" is hundreds of lines long and needs manual maintenance every time new fields are added to the pojo super-class. This is precisely the manual work PB is designed to avoid and negates much of its benefits :( |
This allows
public class AppleBuilder extends FruitBuilderto be generated and, provided all withMethods override, used as an abstract
FruitBuilder.It's ugly but effective. One day it can be followed up with a change that generates only an interface from an abstract pojo.