Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

Enforce setting of Optional fields #191

Open
pgr0ss opened this issue Sep 13, 2016 · 7 comments
Open

Enforce setting of Optional fields #191

pgr0ss opened this issue Sep 13, 2016 · 7 comments

Comments

@pgr0ss
Copy link

pgr0ss commented Sep 13, 2016

What do you think about an option to force consumers of a builder to explicitly set optional fields. For example:

@FreeBuilder
public interface Person {
  Optional<String> getFavoriteColor();
}

Person person = new Person.Builder()
    .setFavoriteColor(Optional.empty())
    .build();

Person person = new Person.Builder()
    .build(); // throws an Exception since favoriteColor is not set

This would ensure that all attribute are accounted for, and prevent empty optionals from creeping in accidentally.

@j-baker
Copy link
Collaborator

j-baker commented Sep 13, 2016

Hey! You can do this already if you like, for example:

@FreeBuilder
public interface Person {
    Optional<String> getFavoriteColor();

    class Builder extends Person_Builder {
        @Override
        public Person build() {
            Person person = super.build();
            Preconditions.checkState(getFavoriteColor().isPresent(), "You must set the favorite color");
            return person;
        }
    }
}

does this accomplish what you need?

@hdurer
Copy link
Collaborator

hdurer commented Sep 13, 2016

The problem here is that Optional has two uses in Java: a) as safer
alternative to null, b) to represent data that may not have been provided.
As I understand it, the OP wanted a) and have FB enforce that it is set in
the builder.

2016-09-13 9:44 GMT+01:00 James Baker [email protected]:

Hey! You can do this already if you like, for example:

@Freebuilder
public interface Person {
Optional getFavoriteColor();

class Builder extends Person_Builder {
    @Override
    public Person build() {
        Person person = this.build();
        Preconditions.checkState(getFavoriteColor().isPresent(), "You must set the favorite color");
        return person;
    }
}

}

does this accomplish what you need?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#191 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACp3V2yCF3KeIVfR5Nz4aHueHyq3v6Iks5qpmJagaJpZM4J7KeW
.

@alicederyn
Copy link
Collaborator

In other words, to not default Optional fields to empty. You can already do that manually by adding a missingX field, and overridding setX/merge/build to maintain it. In terms of adding explicit support to FreeBuilder, I would rather support general pluggable behaviour (issue #14) and have this be one plug-in we provide. Will leave this feature request open for voting.

@pgr0ss
Copy link
Author

pgr0ss commented Sep 13, 2016

@ChrisAlice Can you elaborate on your missingX field workaround? I don't think I understand it. Thanks.

@j-baker
Copy link
Collaborator

j-baker commented Sep 13, 2016

so the simplest case would be something like (in the builder) something like:

@FreeBuilder
public interface Person {
    Optional<String> getFavoriteColor();

    class Builder extends Person_Builder {
        boolean isFavoriteColorSet = false;

        @Override
        public Builder setFavoriteColor(String favoriteColor) {
            isFavoriteColorSet = true;
            return super.setFavoriteColor(favoriteColor);
        }

        @Override
        public Builder clearFavoriteColor(String favoriteColor) {
            isFavoriteColorSet = true; // if you want clearing the favorite color to set it to false, you'd need to override the optional and nullable setters as well.
            return super.clearFavoriteColor();
        }

        @Override
        public Person build() {
            Preconditions.checkState(isFavoriteColorSet, "must set the favorite color to something");
            return super.build();
        }
    }
}

which is pretty unpleasant (looking at JavaUtilOptionalSourceTest.java i'm not sure you need to do anything with merge).

@pgr0ss
Copy link
Author

pgr0ss commented Sep 13, 2016

Yeah, that's not ideal, especially when we have multiple Optional fields on a class. I guess I'll wait for pluggable behavior, then. Thanks.

@alicederyn
Copy link
Collaborator

You'll need to explicitly set isFavoriteColorSet in the mergeFrom(Builder)
method, too (otherwise it will always end up true even if neither builder
had it set beforehand).

On Tue, 13 Sep 2016, 22:04 Paul Gross, [email protected] wrote:

Yeah, that's not ideal, especially when we have multiple Optional fields
on a class. I guess I'll wait for pluggable behavior, then. Thanks.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#191 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA7xqSPSIa0w3buLAWbTDFsuTOBMavvfks5qpw_IgaJpZM4J7KeW
.

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

No branches or pull requests

4 participants