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

[FEATURE] add onBuilder to @Builder #2293

Open
xenoterracide opened this issue Nov 21, 2019 · 9 comments
Open

[FEATURE] add onBuilder to @Builder #2293

xenoterracide opened this issue Nov 21, 2019 · 9 comments

Comments

@xenoterracide
Copy link

xenoterracide commented Nov 21, 2019

Describe the feature

@Builder( onBuilder=@Foo )
class MyClass {
...
@Foo
class MyClassBuilder {

Describe the target audience

this is another potential solution to uber/NullAway#321

@etki
Copy link

etki commented Dec 3, 2019

@xenoterracide a workaround i know is just declare the builder class:

@Builder
class MyClass {
    @Foo
    class MyClassBuilder {}
}

This is practically how people are making Lombok and Jackson work together: https://stackoverflow.com/a/48801237/2908793

But i agree in advance that this is just a workaround, and requested functionality would be nice, since we already have this thing on @*Constructor annotations.

@Maaartinus
Copy link
Contributor

I'm not sure about this. The onConstructor argument is necessary as you don't wont to write a 20-args constructor declaration (and keep it up to date!) to be completed by Lombok. With the builder class, all you need to write is the empty class declaration and the annotation, so that's just a few additional chars. It also allows you to add some methods... Moreover, the on* argument may break in some future Java version, while the workaround probably won't.

So maybe the workaround is the right solution?

@janrieke
Copy link
Contributor

janrieke commented Dec 4, 2019

I tend to agree. However, with @SuperBuilder I see things a bit differently, because the declaration is quite complex, and there are 2 of them.

@randakar
Copy link

randakar commented Dec 4, 2019 via email

@janrieke
Copy link
Contributor

janrieke commented Dec 5, 2019

It's so complex that the documentation says: Use delombok to get it right. And I agree (I wrote it ;-).

@rzwitserloot
Copy link
Collaborator

The problem with using this on @SuperBuilder is that we then need onAbstractBuilder= and onImplBuilder=, which is going a bit too far, I think.

@janrieke
Copy link
Contributor

janrieke commented Jan 15, 2020

Most people would use it for Jackson to deserialize using the builder. For that case, you only need it on the BuilderImpl. On the other hand, there is no need for a Jackson annotation if you can add a prefix. However, you still need to make the BuilderImpl at least package-private for Jackson to use it.

I'm unsure, but right now I would vote for a visibility and prefix option for @SuperBuilder.

@floralvikings
Copy link
Contributor

@janrieke Unless I'm mistaken (@rzwitserloot can confirm or deny) #2174 should be included in the next release, which contains support for prefixing @Builder methods; I haven't looked at @SuperBuilder but that PR might serve as starting point

@janrieke
Copy link
Contributor

janrieke commented Feb 8, 2020

"Set" methods for @SuperBuilder can be prefixed with the next version (PR #2357), making @JsonPOJOBuilder (or setting a custom JacksonAnnotationIntrospector) unnecessary if you specify the prefix in @SuperBuilder.

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

7 participants