Randgalt / record-builder

Record builder generator for Java records
Apache License 2.0
726 stars 51 forks source link

Make builder constructor public #128

Closed toolforger closed 9 months ago

toolforger commented 2 years ago

The purpose is to make record-builder fit for a slightly off-label use case: Generate the getter/setter/equals/hashcode/toString boilerplate for our otherwise standard Java Bean classes. We don't oppose records, actually, it's just that some libraries don't fully support using them yet.

We would like to be able to make a bean class Foo like this:

public class Bean extends FooBaseRecordBuilder {
    // We need a parameterless constructor for a Java Bean.
    // If the parent class has it but makes it private, we can't even create it here.

    // Any overrides for getters/setters go here.
    // (If there are no overrides, maybe we can configure record-builder to create Foo directly.
    // We did not pursue that venue since our team is okay with having an empty Foo class.)
}

where FooBaseRecordBuilder is generated from a FooBase interface:

@RecordInterface
// Actually this is in a @RecordBuilder.Template :-)
@RecordBuilder.Options(booleanPrefix = "is", getterPrefix = "get", setterPrefix = "set")
public interface FooBase {

    boolean isCustomizable();

    @JsonView(Patchable.class) // Probably need a @RecordBuilder.Options entry to have this on the actual Java Bean
    boolean isEnabled();
}

Aside notes:

Randgalt commented 2 years ago

Let's let this sit for a bit to see if others have feedback. Regardless, this should have an option to enable it. It should not be the default.

toolforger commented 2 years ago

Let's let this sit for a bit to see if others have feedback.

Agreed.

Regardless, this should have an option to enable it. It should not be the default.

I was considering it. Thought it's adding to the burden of things one has to configure, so I looked whether it's necessary.

Things that speak against making it configurable:

Things that speak for making it configurable:

My personal take is that we should see if somebody comes up with a guarantee that an unsubclassable builder can give that a subclassable does not. That said, if you think a configuration option is the way to go, I prefer a fast decision even more and will happy add the option.

Randgalt commented 2 years ago

That said, if you think a configuration option is the way to go, I prefer a fast decision even more and will happy add the option.

Yes, please.

toolforger commented 2 years ago

I see that to allow a record builder to be both subclassable and fluent, it needs an additional type parameter for the setters' return type.

This has ramifications for type parameters on Withers, factory methods, and such, but I don't have a useful overview. Should I just add the new type parameter to InternalRecordBuilderProcessor.typeVariables and "it will just work out", or do you expect problems? What generated files should I look at to identify problems? (I suppose somewhere in record-builder-test/target/generated-sources, but which ones?)

yurybubnov commented 1 year ago

I got into a situation when I need a public constructor. I'm working with AWS libraries which are written with Lombok-style generated builders in mind which have a public constructor in the builder and initialize builders via the said constructor. Pretty sure there are other libraries that are tailored towards Lombok-style builders. It would be nice to have the option of making the default, no-arg constructor, public.

Randgalt commented 1 year ago

@yurybubnov when I get time I can enhance this to make it optional, etc. Or maybe you can do it if you can.

KangoV commented 10 months ago

This seems to be hanging around for a while. I'm moving from Immutables.io to this as I need to do some complex Jackson serialisation. In that framework i used to augment the builder with extra helpers, e.g:

public record Grouping(
    UUID id,
    String name,
    String description,
    String context,
    List<String> objectRefs )  {

    public static class Builder extends GroupingBuilder {
        public Builder id(String id) {
            this.id(UUID.fromString(id));
            return this;
        }
    }

    public static Builder build() {
        return new Builder();
    }

}

Just having both constructors on the builder as public or package would mean that this PR solves this use case. Another option for this would be to have a option to make the static builder() and builder(....) methods be package private. This would mean the only method would be the new one in the record itself.

This would mean the builder is "hidden". The immutables.io library does this.

Randgalt commented 10 months ago

tbh - I'm not sure what to do about this. A few bad commits got into RecordBuilder. Thankfully, they're behind options but they are there nonetheless. There's a new PR I haven't looked at. I'm not sure if it addresses this.

KangoV commented 10 months ago

I do think that this should be behind an option, builderConstructorVisibility maybe? The default being private. This would not break existing code.

Randgalt commented 10 months ago

@KangoV can you make a new PR with it behind an option?

KangoV commented 10 months ago

I'll have a go later and see how I get on.

toolforger commented 10 months ago

Anybody please feel free to take over and do as you wish. The project I needed this for and me have parted ways, meaning my feedback wouldn't be very useful anyway. (Also, not getting answers to my latest questions meant I wouldn't be triggered to continue work on this, so that's why there wasn't any progress. No offense taken though.)

Randgalt commented 10 months ago

(Also, not getting answers to my latest questions meant I wouldn't be triggered to continue work on this, so that's why there wasn't any progress. No offense taken though.)

Please accept my apologies for lack of response. The last year or so has left me little time for side projects. I'm trying now to back to this.

toolforger commented 10 months ago

No need to apologize, I know such things happen :-) I just meant to give a heads-up that everybody please feel free to continue as is best for them, and an explanation that it's not anybody's fault.

KangoV commented 9 months ago

Should the default be public? I've had a look through the immutables.io project which I have been using and there is no option to make it private. Do you think an option is needed. I leaning towards not having one, unless someone shouts.

toolforger commented 9 months ago

It should be public if both apply:

I believe both are true.

You may be reluctant to enable new use cases, since more varied use cases means more design constraints which means more design work and possibly more difficult design trade-offs.

I don't see big trouble ahead in this case, so I'd do it if it were my project. On the other hand I don't see all use cases already in existence, and how narrowed-down the design space already is, so my instincts may be working with incomplete data.

Randgalt commented 9 months ago

I'd still like to be controlled by an option. This ship has sailed and I don't want to disrupt any existing use-cases.

toolforger commented 9 months ago

Not wanting to argue that the decision is already made... but making a constructor public isn't going to break any existing code, is it?

Randgalt commented 9 months ago

You'd be surprised. This is more of a gut reaction based on experience. I've made what I thought were innocuous changes in this and other libraries only to be surprised by the reaction of the community.

Randgalt commented 9 months ago

Replaced by https://github.com/Randgalt/record-builder/pull/160