Don't use @ConstructorProperties for method selection

[drekbour]

The annotation already has a purpose and is neither mandatory nor unique within a given class (PB picks the first such annotated constructor). However, if present, it should always be used for property naming.

Changing this means:
a. Something else must be used to explicitly identify a constructor (we could default if there is only one?)
b. implicit naming is possible

I'd suggest either moving @GeneratePojoBuilder to the constructor level or creating a new annotation specifically for the purpose. It would need to be fair to existing code usages though unless we released this went into a v3 roadmap

Up for discussion as I have not really thought through it completely...

[mkarneim]

Yes, I think this could be done (as long as it is consistent with issue #42).

So the rule would be something like this:

"A PojoBuilder will be generated, if there exists exactly one @GeneratePojoBuilder annotation inside the pojo's class either on class level or constructor level.

If the annotation is on class level ...

• and there is only one constructor, then that on is used inside the build() method
  • if it has parameters, then implicit parameter naming is done like mentioned in issue @PropertyNames/@FactoryProperties could be optional #42.
• if many constructors exist ...
  • and there is an empty constructor, then that one is used.
  • otherwise a specific annotation is required on the constructor to select it."

Concerning the last point I agree this could be the @GeneratePojoBuilder annotation itself. I just don't like it very much that the programmer must search the whole class for this annotation just to find out that another programmer has already annotated it. It's somehow a question of readability. But I am not sure how important this is...

What do you think?

[drekbour]

I hadn't considered allowing it at either (not both) class or constructor. As far as I'm concerned that's a good solution 👍

I worry slightly about defaulting to the empty constructor when others exist as this could easily be the wrong choice. In the following example, there is only one way to set the name property:

@GeneratePojoBuilder
public class Thing {
  private final String name;
  public Thing() {
    this( "DEFAULT" );
  }
  public Thing(String name) {
    this.name = name;
  }
  public String getName()
  ...

Now the user could simply move the @GeneratePojoBuilder annotation to resolve this but I'd imagine it would confuse them whilst figuring out what was going on when ThingBuilder.withName() disappeared because someone added an empty constructor. Perhaps I'm seeing too many problems.

I'd consider making implicit naming on constructor parameters a new feature seperate to this one (one step at a time!).

[mkarneim]

You are right.
Defaulting to the empty constructor is a bad idea, except if it is the implicit default constructor (without custom code).

That leaves us with the question: which annotation to use for identifying the constructor that we use in the build() method. It looks like @GeneratePojoBuilder is the right choice since it resembles the way we identify the factory method.

[drekbour]

In doing this we also need to support the existing usecase (@GeneratePojoBuilder at class level, single @ConstructorProperties annotation). I think that should be "deprecated" though and all forward usage is done by annotating a specific constructor (#47 will require this or become full of fragile guess work as to which constructor to use)