Skip to content
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

Closed
carlzogh opened this issue Aug 4, 2018 · 17 comments
Closed

Add option to define a prefix for @Builder methods #1805

carlzogh opened this issue Aug 4, 2018 · 17 comments

Comments

@carlzogh
Copy link

carlzogh commented Aug 4, 2018

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.

@rzwitserloot
Copy link
Collaborator

'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.

@florianscharinger
Copy link

@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.

@Maaartinus
Copy link
Contributor

@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 withX(5), you don't change anything, you just get an object "having x=5".

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 (@Wither), so we really should stick with them. I could imagine a normal Builder using the "set" prefix and another Builder using the "with" prefix and cloning itself (though the latter would be rather inefficient in most cases).

@aebaugh
Copy link

aebaugh commented Sep 7, 2018

@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.

@janrieke
Copy link
Contributor

janrieke commented Sep 8, 2018

You could simply add @JsonPOJOBuilder(withPrefix = "") to the builder class. One could argue for a parameter onBuilder for @Builder (similar to the onConstructor parameter for @*ArgsConstructor) to avoid manually writing the builder class definition. But even without it, I would never say that "it's a pain".

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".
Making it configurable seems to be no big deal at first glance; however, such a feature has to be supported and maintained for the rest of lombok's lifetime, and for both @Builder and @SuperBuilder. Lombok is not a project backed by a large company or a larger number of maintainers, so adding such a feature must be well-considered. It just seems not worth it right now.

@aebaugh
Copy link

aebaugh commented Sep 10, 2018

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.

@OndraZizka
Copy link

I am a bit confused. Lombok team is against having builder methods with a prefix, e.g. set?
For instance, Foo.builder().setName("Ondra").build() - that's something you don't want to support?

@MrXo
Copy link

MrXo commented Dec 12, 2018

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
https://docs.oracle.com/javase/8/docs/api/java/util/Calendar.Builder.html

@OndraZizka
Copy link

Right. But there's a bigger case for allowing prefixes, specifically set:
Refactoring (replacing constructor with a builder and vice versa) is much easier when the methods have the same names. I have done this on around 120 classes and believe me - it would make a big difference. Esp. for the PR reviewers :)

@Sam-Kruglov
Copy link

Sam-Kruglov commented Feb 19, 2019

@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.

@jiridj
Copy link

jiridj commented Apr 5, 2019

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.

@spaceCamel
Copy link

Just another one here that would have liked to add a prefix onto builder methods.
We have a massive amount of builders using the 'with' prefix. We would be so glad to replace them with Lombok-generated ones. But the builders are invoked from groovy tests and being unable to specify a prefix means we would have to rename every single method on the builders to align the groovy tests before we can replace them with @Builder. This is going to be more work than what my company can afford at the moment, I'm afraid. Also it would force us to change test-code when it was not strictly necessary which is added risk and cost to the equation.

We would also like to combine Jackson and Lombok @Value classes. It's not that bad to add the @JsonPOJOBuilder annotation on the Builder, but it's also not that clean and straightforward.

I agree that using "with" prefix on builders was not a great idea, yet that's the code we have to work with.

@tommc
Copy link

tommc commented May 17, 2019

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

@epalumbo
Copy link

This is a very good feature to have, could it be reopened and further discussed?

@rzwitserloot
Copy link
Collaborator

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.

@Sam-Kruglov
Copy link

Thanks! 🙂

@maciejkopecpl
Copy link

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests