Add Default value #31

MehrunesSky · 2024-11-28T16:18:18Z

Hello,

I would like to add default values in JIlt without having to specify them in my constructor.
So I thought of an @Builder.Default annotation a bit like lombok, which takes the default value as a parameter.
This is just a suggestion. If you have any feedback or another idea for implementing this, please don't hesitate to contact me.

skinny85 · 2024-11-29T14:57:43Z

Hi @MehrunesSky,

Thanks for submitting the PR!

Before I review it in depth, can you share more about the use case here? In what situation would you want to have a default value only in the Builder, and not in the constructor of your class?

Thanks,
Adam

MehrunesSky · 2024-11-30T09:03:21Z

Hello,

I have very large objects and jilt allows me to easily instantiate them with properties that are optional and others that are not.
But as a consequence, my constructor is very consequent just because I have to manage default values (like initialized with empty collections for example). I'd like to use lombok's @AllArgsConstructor and if I'd used lombok's @builder, I'd have used @Builder.Default.
And that's the functionality I'd like with Jilt, to be able to indicate that if an attribute hasn't been entered, then I take the default value.

I don't know if it's clear, please don't hesitate if I need to clarify.

skinny85 · 2024-12-01T08:07:03Z

Thanks for the explanation! I suspected this is related to some Lombok BS 😉.

I was thinking of a slightly different design for this feature 🙂. Since having a default value implies the property is optional, I wouldn't introduce a new annotation just for this, but instead use the existing @Opt annotation.

Another part I'm not super enthusiastic about is the initializer being inside a string. I'd much rather prefer if it was regular code.

Given that, how about we change the implementation to instead take the Builder initializer from the field initializer, same way as Lombok's @Builder.Default works?

So, something like this:

import lombok.AllArgsConstructor;
import org.jilt.Builder;
import org.jilt.Opt;

@AllArgsConstructor
@Builder
public final class MyClass {
    @Opt
    private String field = initializeMe();

    // ...
}

This would result in field being initialized with initializeMe() in MyClassBuilder.

Alternatively, if you're worried about this change not being backwards compatible, I would also accept special-casing the treatment of Lombok's @Builder.Default instead of introducing our own. We would treat all fields annotated with @lombok.Builder.Default as implicitly optional, similarly like we do for @Nullable annotations today, and we would copy over the initializer to the Builder, so it would look something like:

import lombok.AllArgsConstructor;
import lombok.Builder.Default;
import org.jilt.Builder;

@AllArgsConstructor
@Builder
public final class MyClass {
    @Default
    private String field = initializeMe();

    // ...
}

Note that we would have to check for the presence of @Builder.Default dynamically, because I don't want to add a Lombok dependency to Jilt (you can check how we do it for @Nullable annotations here).

Let me know what you think of these ideas!

MehrunesSky · 2024-12-01T13:50:13Z

It's a great idea to use lombok's Builder.Default.
I had thought of using the annotated field initializer, but I don't really see how to implement it.

Do you have any advice on how to implement it? Using a third-party dependency like ASM or Javassist? Or I'm thinking of initializing the Class and retrieving the value by reflection, but that seems a bit complex.

I've never really used Java reflexion, so it's quite new for me to contribute to a code generation project.

skinny85 · 2024-12-01T16:29:44Z

I would just copy the initializer code as-is, and make sure to clarify in the documentation that you have to use fully-qualified names if you use anything that requires an import (like your example with Collections.emptySet() in the test) 🙂.

MehrunesSky · 2024-12-01T18:13:04Z

But how do you copy the initializer as-is without external dependencies? Is there a technique? Do you have an example?

skinny85 · 2024-12-01T18:23:50Z

Like I said, you ignore external dependencies, and copy the code as-is. Which means you need to use fully-qualified names instead of imports. Exactly like you do right now to reference Collections.emptySet().

MehrunesSky · 2024-12-01T21:02:28Z

I don't quite understand.
To sum up, you have to use the @lombok.Builder.Default annotation and take the default initializer.

So if the class is like this :

@AllArgsConstructor
@Builder
public final class MyClass {
    @lombok.Builder.Default
    private String field = initializeMe();

    // ...
}

The builder will look something like this:

public final class MyClassBuilder {

    private String field = initializeMe();

    // ...
}

But I can't see how to technically copy the initializeMe() to the builder. I don't think I can just grab the “initializeMe();” with the java api.
I'm really sorry I'm having trouble understanding.

skinny85 · 2024-12-01T21:28:40Z

So, in a test in your PR, you have:

import org.jilt.Builder;

@Builder
public class DefaultValue {
    @Builder.Default("java.util.Collections.emptySet()")
    public Set<String> attrs;

    // ...
}

Notice it's "java.util.Collections.emptySet()", not just "Collections.emptySet()".

If we want to be consistent with how Lombok's @Builder.Default works, and implement the changes I suggested above, that would force the code to be this instead:

import lombok.AllArgsConstructor;
import lombok.Builder.Default;
import org.jilt.Builder;

@AllArgsConstructor
@Builder
public class DefaultValue {
    @Default
    public Set<String> attrs = java.util.Collections.emptySet();

    // ...
}

Similarly, with initializeMe() above, assuming initializeMe() was a static method on MyClass, you would have to spell it out explicitly to make it work with my proposal, so you would have to change your code to:

import lombok.AllArgsConstructor;
import lombok.Builder.Default;
import org.jilt.Builder;

@AllArgsConstructor
@Builder
public final class MyClass {
    @Default
    private String field = MyClass.initializeMe(); // just initializeMe() wouldn't work here

    // ...
}

And if initializeMe() was instead statically imported from a different class, you would have to use a fully-qualified reference to make it work:

import lombok.AllArgsConstructor;
import lombok.Builder.Default;
import org.jilt.Builder;

@AllArgsConstructor
@Builder
public final class MyClass {
    @Default
    private String field = com.mycompany.otherpackage.MyOtherClass.initializeMe();

    // ...
}

And we would explain all of these requirements in the documentation.

Does this make sense? 🙂

MehrunesSky · 2024-12-01T22:15:40Z

Okay, I understand better. But do you have any idea how to implement your proposal? It seems very complicated to implement.

skinny85 · 2024-12-01T22:58:14Z

Why? 🙂 Like I said before, just copy the initializer as-is from the class to the Builder, and you're done 🙂.

…and Opt

MehrunesSky · 2024-12-02T21:45:23Z

Hello,

I've just updated the code to take your suggestion in consideration. Is it pretty much what you had in mind? I haven't updated the doc yet, I'll update it when it's good on the code part.
However, the problem with the code I've provided is that it doesn't work with Java 8, because Trees isn't available.

skinny85

Thanks a lot for the new revision @MehrunesSky! I have a few comments, most importantly, we need to make sure that, if a field is annotated with @lombok.Builder.Default, it is treated as optional (same as if it was annotated with @Opt).

skinny85 · 2024-12-03T04:53:41Z

src/main/java/org/jilt/Builder.java

@@ -333,4 +333,5 @@
    @Retention(RetentionPolicy.SOURCE)
    @interface Ignore {
    }
+


No need for this change I think 🙂.

skinny85 · 2024-12-03T04:55:40Z

build.gradle

@@ -100,6 +100,7 @@ dependencies {
    compileOnly 'com.squareup:javapoet:1.8.0'

    testImplementation 'junit:junit:4.13.2'
+    testImplementation 'org.projectlombok:lombok:1.18.36'


I think we need Lombok also in the testAnnotationProcessor configuration?

It's fix :)

Do we need Lombok in both testImplementation and testAnnotationProcessor? Isn't testAnnotationProcessor enough?

src/main/java/org/jilt/internal/AbstractBuilderGenerator.java

skinny85 · 2024-12-03T05:33:08Z

src/main/java/org/jilt/internal/AbstractBuilderGenerator.java

@@ -33,7 +36,11 @@
 import java.util.List;

 abstract class AbstractBuilderGenerator implements BuilderGenerator {
+
+    private static final CodeBlock CODE_BLOC_EMPTY = CodeBlock.of("");


I don't think we need this field. I think it will be used only once. I would remove it.

skinny85 · 2024-12-03T05:40:15Z

src/main/java/org/jilt/internal/AbstractBuilderGenerator.java

+            CodeBlock initializer = CODE_BLOC_EMPTY;
+            if (attribute.getAnnotation(Opt.class) != null || getLombokBuilderDefaultAnnotation(attribute.getAnnotationMirrors())){
+                initializer = getCodeBlockInitializer(attribute);
+            }
+


I'm not a huge fan of this code structure, given that getCodeBlockInitializer() can also return an empty CodeBlock.

So, let's change the code to be just:

Suggested change

CodeBlock initializer = CODE_BLOC_EMPTY;

if (attribute.getAnnotation(Opt.class) != null || getLombokBuilderDefaultAnnotation(attribute.getAnnotationMirrors())){

initializer = getCodeBlockInitializer(attribute);

}

CodeBlock initializer = this.builderFieldInitializer(attribute);

And at that point, the code is so simple, I would just inline the initializer variable, and get rid of it completely.

(If it wasn't clear, I'm also not a huge fan of the getCodeBlockInitializer name 😛)

skinny85 · 2024-12-03T06:03:39Z

src/test/java/org/jilt/test/data/defaultvalue/DefaultValueWithLombok.java

+    public int attr4 = 1000;
+
+    @lombok.Builder.Default
+    public int attr5 = 1000;


Let's try referencing the field here, to check if that works? So something like

Suggested change

public int attr5 = 1000;

public int attr5 = attr1 + 1000;

skinny85 · 2024-12-03T06:05:01Z

src/test/java/org/jilt/test/data/defaultvalue/DefaultValueWithOpt.java

+    @Opt
+    public int attr1 = 1;


Let's not implement this functionality for @Opt. Let's make it work only for @lombok.Builder.Default.

I would remove this entire class.

skinny85 · 2024-12-03T06:06:16Z

src/test/java/org/jilt/test/data/defaultvalue/DefaultValueWithLombok.java

+
+@Builder
+public class DefaultValueWithLombok {
+


Suggested change

skinny85 · 2024-12-03T06:09:49Z

src/test/java/org/jilt/test/data/defaultvalue/DefaultValueWithLombok.java

+
+import java.util.Set;
+
+@Builder


This PR is missing the functionality of marking a field annotated with @lombok.Builder.Default as optional. We would catch it if we made the @Builder here STAGED.

So, let's change this to:

Suggested change

@Builder

@Builder(style = BuilderStyle.STAGED)

and make sure the DefaultTest test still passes.

skinny85 · 2024-12-03T06:10:54Z

src/test/java/org/jilt/test/DefaultTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class DefaultTest {


Let's call this class LombokDefaultTest, or something like that, instead.

Co-authored-by: Adam Ruka <[email protected]>

add default value

4059177

add default value

26f1d2f

MehrunesSky force-pushed the master branch from 0f380ff to 26f1d2f Compare November 30, 2024 09:48

Use default initializer on field annoted with Lombok builder default …

a72bd5d

…and Opt

skinny85 reviewed Dec 3, 2024

View reviewed changes

MehrunesSky and others added 2 commits December 4, 2024 08:00

Apply suggestions from code review

768f31e

Co-authored-by: Adam Ruka <[email protected]>

Apply suggestions from code review

9a97cc3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Default value #31

Add Default value #31

MehrunesSky commented Nov 28, 2024

skinny85 commented Nov 29, 2024

MehrunesSky commented Nov 30, 2024

skinny85 commented Dec 1, 2024 •

edited

Loading

MehrunesSky commented Dec 1, 2024

skinny85 commented Dec 1, 2024

MehrunesSky commented Dec 1, 2024

skinny85 commented Dec 1, 2024

MehrunesSky commented Dec 1, 2024

skinny85 commented Dec 1, 2024

MehrunesSky commented Dec 1, 2024

skinny85 commented Dec 1, 2024

MehrunesSky commented Dec 2, 2024

skinny85 left a comment

skinny85 Dec 3, 2024

skinny85 Dec 3, 2024

MehrunesSky Dec 4, 2024

skinny85 Dec 5, 2024

skinny85 Dec 3, 2024

skinny85 Dec 3, 2024 •

edited

Loading

skinny85 Dec 3, 2024

skinny85 Dec 3, 2024

skinny85 Dec 3, 2024

skinny85 Dec 3, 2024 •

edited

Loading

skinny85 Dec 3, 2024


		import static org.assertj.core.api.Assertions.assertThat;

		public class DefaultTest {

Add Default value #31

Are you sure you want to change the base?

Add Default value #31

Conversation

MehrunesSky commented Nov 28, 2024

skinny85 commented Nov 29, 2024

MehrunesSky commented Nov 30, 2024

skinny85 commented Dec 1, 2024 • edited Loading

MehrunesSky commented Dec 1, 2024

skinny85 commented Dec 1, 2024

MehrunesSky commented Dec 1, 2024

skinny85 commented Dec 1, 2024

MehrunesSky commented Dec 1, 2024

skinny85 commented Dec 1, 2024

MehrunesSky commented Dec 1, 2024

skinny85 commented Dec 1, 2024

MehrunesSky commented Dec 2, 2024

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Dec 1, 2024 •

edited

Loading

skinny85 Dec 3, 2024 •

edited

Loading

skinny85 Dec 3, 2024 •

edited

Loading