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

@Builder.Constructor does not support all @Value.Style parameters #1444

Open
ewirch opened this issue Mar 2, 2023 · 2 comments
Open

@Builder.Constructor does not support all @Value.Style parameters #1444

ewirch opened this issue Mar 2, 2023 · 2 comments

Comments

@ewirch
Copy link

ewirch commented Mar 2, 2023

Documentation of @Builder.Constructor claims:

Class level and package level style annotations fully supported (see org.immutables.value.Value.Style).

But this is not true. Many Style parameters are not supported. Here are some, which have no effect:

  • stagedBuilder=true
  • allMandatoryParameters=true
  • allParameters=true

Code sample to reproduce:

@Value.Style(allParameters = true)
public class Person {
    private String name;
    @Nullable private String nickName;

    @Builder.Constructor
    public Person(String name, @Nullable String nickName) {
        this.name = name;
        this.nickName = nickName;
    }
}

Generated constructor:

  public PersonBuilder() {
  }

Using Immutables 2.9.3.

@elucash
Copy link
Member

elucash commented Mar 2, 2023

Sorry for the wording, "fully supported" means that type/package/meta style annotation will be applied, not that all combination of styles are supported. Staged builder always had limitations, need to check. As about allParameters and allMandatoryParameters attributes, I believe those are supposed work differently that expected in the example. all[Mandatory]Parameters is about turning all attributes of Value.Immutable interface or abstract class into constructor parameters (i.e. .of(a,b) or new Person(a,b)) not builder parameters, i.e. has nothing to do with @Builder.Parameter behavior. As @Builder.Constructor already starts with constructor, there is no way all[Mandatory]Parameters can be applicable. But you still can use @Builder.Parameter on constructor parameters to propagate those to the constructor of a generated builder. Even, hypothetically, having attributes like allBuilderParameters would make no sense as why would anyone need new Builder(a, b, c, d).build() instead of new Cons(a, b, c, d) ?

@ewirch
Copy link
Author

ewirch commented Mar 3, 2023

Thanks for the quick reply @elucash.
IMO allMandatoryParameters is about required (versus optional) attributes. In other words: those attributes, which the compiler forces you to provide, when creating an instance of that object. For @Value.Immutable this is the set of constructor parameters. For @Builder.Constructor it is the set of builder constructor parameters.
The net effect is: when you change the set of required attributes, code compilation fails. This is what we want.

as why would anyone need new Builder(a, b, c, d).build() instead of new Cons(a, b, c, d) ?

I agree, it makes no sense for allParameters=true. But it makes a lot of sense for allMandatoryParameters, when c and d are optional. The (intended) code would look like

Cons(Object a, Object b, @Nullable Object c, @Nullable Object d) {
ConsBuilder(Object a, Object b) {
new ConsBuilder(a, b)
  .c(newC)
  .d(newD)
  .build();

// or
new ConsBuilder(a, b)
  .c(newC)
  .build();

Background: in our company, we consider using builder pattern in production code to be an anti pattern. The reason is, that the compiler doesn't warn you, when a required attribute is added to the target class. This is different when using constructors. A constructor requires to pass all required attributes. But when optional attributes enter the stage, and maybe many of them, constructors become harder to use. When we could change the builder pattern, that the compiler forces you to provide required attributes, we could use builder pattern in production code. This is what staged builder does. The same would be possible when @Builder.Constructor would support allMandatoryParameters=true.

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

No branches or pull requests

2 participants