Add optional prefixes to Builder setter method names

[floralvikings]

Related to #1805, this change adds an optional setterPrefix parameter
to the Builder annotation; if this parameter is unspecified or blank
the behavior of the Builder annotation is unchanged, but if it is
present the value specified will be prefixed to the generated methods.

For example, using:

@builder(setterPrefix = "include")
class Foo {
    private int someValue;
}

will result in a generated Builder class containing an includeSomeValue(int someValue)
method instead of the default someValue(int someValue).

[floralvikings]

@rzwitserloot is there anything you'd like to see added to this PR?

[rzwitserloot]

I wish you talked to us first; what's the use case?

By far the most common requests we've heard is for 'setX' and 'withX', and we think both of these are objectively bad: 'with' is a term that ought to be reserved for pure, immutable updaters (a builder is the opposite of pure/immutable, hence, a misnomer), and 'set' suggests it's a POJO/bean; builders aren't, hence a misnomer.

This include business is new and even stranger. Why would you want your 'builder-setter' method to be prefixed with 'include'?

This isn't a case of "hey, if it does not help, at least it doesn't hurt": But it DOES hurt; this is a parameter on the annotation. I can cook up 49 other exotic weird things that we can't explain, at which point @Builder would have 50 parameters, and surely you can agree that it'd be a worse feature if it had that.

If there is a solid use-case, then maybe this can be a feature, but it probably needs a lombok.config key, and certainly needs tests in test/transform/resource and updated docs in the website directory.

But most of all, this needs an explanation for why; without a solid use case we have to deny the PR.

[floralvikings]

By far the most common requests we've heard is for 'setX' and 'withX', and we think both of these are objectively bad: 'with' is a term that ought to be reserved for pure, immutable updaters (a builder is the opposite of pure/immutable, hence, a misnomer), and 'set' suggests it's a POJO/bean; builders aren't, hence a misnomer.

I don't disagree on any point; my use case is for working with legacy and/or third-party conventions that don't match those used by Lombok (they use withXYZ). In this particular case, we have a very large number of pre-existing model and builder classes spread across multiple repositories. We'd like to use Lombok to generate our builders going forward, but doing so would mean either

1. Having two different conventions for builders, depending on whether they were written before or after we began using Lombok

or

2. Refactoring the existing legacy code to match the Lombok conventions

Both of these are non-starters given the number and spread of pre-existing builders, and I don't think this situation is particularly uncommon for other teams.

This include business is new and even stranger. Why would you want your 'builder-setter' method to be prefixed with 'include'?

This was just an example demonstrating that the prefix can be configured to whatever is needed; I'm not actually suggesting it as a practice or anything.

This isn't a case of "hey, if it does not help, at least it doesn't hurt": But it DOES hurt; this is a parameter on the annotation. I can cook up 49 other exotic weird things that we can't explain, at which point @builder would have 50 parameters, and surely you can agree that it'd be a worse feature if it had that.

Right; I'm not suggesting adding "exotic weird things" or a large number of parameters or anything like that. This addition is intended to help those of us working with large pre-existing or third-party code bases that don't follow ideal conventions and can't be feasibly updated to do so.

[floralvikings]

Regarding:

...it probably needs a lombok.config key, and certainly needs tests in test/transform/resource and updated docs in the website directory.

This PR already has tests in test/transform/resources:

• test/transform/resource/after-delombok
• test/transform/resource/after-ecj
• test/transform/resource/before

I didn't make any other changes (to docs or what have you) as I wasn't certain this PR would be accepted, but I'd be more than happy to do so if you decide this is a desirable feature.

[topr]

@rzwitserloot first of all huge kudos for the Lombok.

I'm surprised you still keep asking about use case / justification after it was given multiple times under #1805 and here. Let me sum this up:

1. adding Lombok's @Builder to legacy code without needing to refactor the whole codebase
2. being able to use property convention with Groovy and Kotlin to set properties on Lombok's generated builder objects (BTW. Groovy's @Builder annotation allows this from its very beginning).
3. because the worry of 50 weird params on @Builder in this case seems to be quite an exaggeration (although I appreciate the concern).

Anyway, it'd be nice to see this accepted.

[rzwitserloot]

Okay, all. We held a long debate, and, the legacy support thing won out. We'll allow it. However, a few notes:

1. Please don't forget to add your name to the AUTHORS file
2. Can you fix up singulars? We talked about it, and the obvious would be that, if you have a singular-ized field 'names', that it'd be withNames, withName and clearNames (versus the prefix-free name, names, clearNames.
3. Unfortunately this PR crossed with some other builder updates, so there is now a conflicted. Fortunately clearing this conflict isn't too difficult; I assume you can do it. If you need help let me know.
4. If you want to write the doc updates (both in 'website' and 'doc/changelog') by all means. But we can do that too if you prefer to leave it.
5. This feature should solely be a parameter on the @Builder annotation which is exactly how you designed it. The javadoc should explicitly call out that using the feature to prefix with is discouraged, as with normally suggests immutable data structures, and builders are the opposite of that. We can take care of that too if you don't want to do the writing.

[floralvikings]

@rzwitserloot thanks for taking the time to consider this PR and provide feedback! I appreciate that this probably wasn't an easy decision to come to.

I'll take care of all the changes you requested; for prefixing singulars, I was thinking of adding a new setterPrefix field to the EclipseSingularRecipes.SingularData class and then using that in the Eclipse[Foo]Singularizer implementation to prefix the generated methods.

So for the EclipseJavaUtilListSetSingularizer class, in the generateSingularMethod method, I'd change the method generation code from this:

md.selector = fluent ? data.getSingularName() : HandlerUtil.buildAccessorName("add", new String(data.getSingularName())).toCharArray();

to this:

char[] prefixedSingularName = data.getSetterPrefix().length == 0 ? data.getSingularName() : HandlerUtil.buildAccessorName(new String(data.getSetterPrefix()), new String(data.getSingularName())).toCharArray();
md.selector = fluent ? prefixedSingularName : HandlerUtil.buildAccessorName("add", new String(data.getSingularName())).toCharArray();

Would that be acceptable? I haven't spent much time looking at singulars or how the code behind them is generated so if I'm just way off-base please don't hesitate to let me know 😅

[floralvikings]

@rzwitserloot I've duplicated all of the existing Builder tests, including a setterPrefix and modifying the inputs and outputs accordingly; sorry it took so long to get this together!

I think I've covered every case in which the setterPrefix might be used but I'm happy to add more tests on request.

If the code changes look good to you I'll go ahead and get started on the docs.

[floralvikings]

@rzwitserloot just checking in to make sure this PR is still something you'd be willing to bring in; if the code changes look alright I'll write up the docs

[rzwitserloot]

Yup, still active. We've been swamped with presentations (3 separate once in a span of a month) whilst @rspilker had lots of business trips planned and I'm in the middle of a very busy time of the year at my dayjob too. No new lombok version is going to be released without this unless something very drastic happens.

[rzwitserloot]

Updated a bunch of stuff and I think I'm going to eliminate some of these tests, I think you went a tad overboard :)

I'll soon push out an edge release with this; next stable lombok release will have it (We'll try to get a move on; current stable lombok has some issues with JDK13 which this edge release fixes...)

[floralvikings]

@rzwitserloot Awesome! I definitely went overboard on the tests, but I wanted to be sure I wasn't missing any use cases because I missed quite a few with my first attempt 😅

Please let me know if there's anything other input/assistance you'd like on this, and thanks again for taking the time to review, respond and merge (and for all the other work you do maintaining this awesome project😄)

[lithium147]

Wow, I cant believe you guys finally came off your high horse and implemented the most requested feature in lombok.. lol