-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to define a prefix for @Builder methods #1805
Comments
'with' is, by convention of course, something you use to indicate you get a clone with 1 updated value. Builder does not clone, and we don't want to create the impression that builder is something it isn't. There's no use for this 'prefix' idea, except for withX. I know this is probably not what you want to hear, but, you'll have to make the case to your team that your convention sucks. |
@rzwitserloot, I am curious to understand why 'with' indicates a clone. The common convention to use 'with' for builder methods is, by convention, read as "creating a builder with value x and with value y", implying that the builder object stores/contains these values. https://en.oxforddictionaries.com/definition/with uses terms like 'accompanied', 'having', or 'possessing'. None that indicate cloning. |
@florianscharinger I guess, this conventions comes from functional programming, rather than from English. Anyway, all meanings you listed indicate no action, especially no change. And that's the point, by calling Cloning isn't indicated as it's implied as the only way how to get an object "possessing x=5" without side effects. Anyway, these conventions are already used by Lombok itself ( |
@rzwitserloot The problem is not when "my" convention sucks, it is when a convention has been adopted by many in the industry, thus making lombok integration hard with common/complimentary/legacy software that could increase its adoption. Case in point, I think lombok+jackson is a pretty killer combination for writing minimal code, and both mostly support better patterns than public, no-arg constructors and universal setters. The inability to specify what the resulting builder setter method prefix (or lack thereof) will be makes this integration with jackson a pain, and also seems an arbitrary line to draw in lombok configurability. It is already quite flexible, but then you hit "the thing" that makes adopting it for your team/company/application/integration problematic, and it seems like many feature requests or pull requests in this area over the years have been declined on mostly semantic grounds. |
You could simply add I just did a quick google-based comparison. Although there seems to be a slight prevalence for the "set prefix" convention, there are also many examples of "no prefix". The least number of examples seem to use the "with prefix". |
Thanks @janrieke, I completely appreciate your last point about maintainability, the longest phase of most features. In my particular example with jackson, your suggestion is exactly what we have done, and requires per class cruft we can mostly otherwise remove with lombok. Thought I'd add a plug in case another submission like #1066 comes up. |
I am a bit confused. Lombok team is against having builder methods with a prefix, e.g. set? |
I find it pretty weird that the convention is said to be without prefixes if the API itself has examples with a prefix like Locale and Calendar Builder: https://docs.oracle.com/javase/7/docs/api/java/util/Locale.Builder.html |
Right. But there's a bigger case for allowing prefixes, specifically |
@rzwitserloot I also vote for reopening: if I could add a "set" prefix, I could use a property syntax in my Kotlin tests. @Builder(prefix = "set")
class Thing{
private final String id;
} fun thing(init: Thing.ThingBuilder.() -> Unit): Thing {
val builder = Thing.builder()
init(builder)
return builder.build()
}
val newThing = thing {
id = "1"
}
//how it looks without prefix
val newThing = thing {
id("1")
} Although not everyone writes tests in a different language, so I am not sure it's enough of a reason. |
I agree with @Sam-Kruglov! Adding a prefix option to the Builder annotation is definitely cleaner than having to add empty boilerplate code just to add Jackson's JsonPojoBuilder annotation to the generated builder. That leaves the naming convention of the builder methods to the developer, which is the lesser of two evils IMO. |
Just another one here that would have liked to add a prefix onto builder methods. We would also like to combine Jackson and Lombok I agree that using "with" prefix on builders was not a great idea, yet that's the code we have to work with. |
Another vote to re-open this please. In trying to make the case that our team's convention sucked, the team instead concluded that the lombok devs have horrible attitudes |
This is a very good feature to have, could it be reopened and further discussed? |
Good news everyone – made it after all, and you owe @floralvikings a beverage or two for pulling the cart on this one! Will be in next stable, and is in the current edge release. |
Thanks! 🙂 |
@rzwitserloot First, thanks a lot! I need this to remove the boilerplate code (existing project). The previous team started using builders with with prefix, and doing refactoring of all occurrences would take forever. But with this, I can configure the builder in such a way that I can use Lombok and don't change the usage of a builder in an existing codebase. Quick win-win situation :) Second, oo you know when it will end up in the stable pipeline? |
Would it be possible to add a "prefix" option for the
@Builder
annotation so that:@Builder(prefix = "with") public class MyModel { private final String myString; private final int myInt; }
Produces builder methods:
MyModel.builder().withMyString(stringVar).withMyInt(intVar).build()
As opposed to currently being limited to:
MyModel.builder().myString(stringVar).myInt(intVar).build()
This will help teams with conventions that use the "with" prefix for builder classes adopt the library and use it.
The text was updated successfully, but these errors were encountered: