Options appearing wierdly in builder

[bowbahdoe]

When I have an Option as a field in a data structure and I put both @Value.Immutable and @VavrEncodingEnabled, the builder that is generated requires knowing that the field is optional when constructing it. For example

@Value.Immutable
@VavrEncodingEnabled
public interface Example {
    Option<String> thing();
}

generates a builder like this

public final Builder from(Example instance) {
  Objects.requireNonNull(instance, "instance");
  thing(instance.thing());
  return this;
}

public Builder thing(Option<String> opt) {
  this.thing_optional = opt;
  return this;
}

public Builder setValueThing(String x) {
  this.thing_optional = Option.of(x);
  return this;
}

public Builder unsetThing() {
  this.thing_optional = Option.none();
  return this;
}

The what I want is for the thing method in the builder to be the same as the setValueThing method, which makes it invisible to the consumer building theImmutableThing that the data structure uses an Option.

Also, feel free to correct any misconceptions I might have about how this should work; at the end of the day not using setValueX might just be preference, but I personally think that it is an interesting design choice.

[io7m]

I think this mostly comes down to a matter of taste: I'm not a fan of method overloading. I think it was a design mistake in Java and so tend to avoid it everywhere. I'm not sure if adding an overloaded method could cause ambiguity here. If you desperately want it, I'll look into adding a thing method that takes T instead of Option<T>.

[bowbahdoe]

I don't exactly disagree, but it does diverge from how Immutables handles the regular optional. For instance, this code

import org.immutables.value.Value;

import java.util.List;
import java.util.Optional;

@Value.Immutable
public interface ValueObject {
    String name();
    List<Integer> counts();
    Optional<String> description();
}

Generates these setters in the builder

    /**
     * Initializes the optional value {@link ValueObject#description() description} to description.
     * @param description The value for description
     * @return {@code this} builder for chained invocation
     */
    public final Builder description(String description) {
      this.description = Objects.requireNonNull(description, "description");
      return this;
    }

    /**
     * Initializes the optional value {@link ValueObject#description() description} to description.
     * @param description The value for description
     * @return {@code this} builder for use in a chained invocation
     */
    public final Builder description(Optional<String> description) {
      this.description = description.orElse(null);
      return this;
    }

[bowbahdoe]

I guess at the end of the day it is surprising because the readme says that Use of Vavr's Option type will receive the same special treatment as the standard Java Optional type. and it is unexpected that they would diverge here.

That isn't to say there isn't merit to not using overloading. I feel like I see where you are coming from on that.

At the very least there must be a good way to document this divergence to make it clear why setValueX is used.

[elucash]

There's definitely some idiosyncrasies on parts of both Immutables and Immutables-Vavr and "same special treatment" may not necessarily mean full API parity. The good thing is that any encoding can serve as an example, where you can create your own with the needed level of API consistency.

[io7m]

I'd like to correct this, but I'm not exactly sure how.

I assumed I could add a method like this:

    @Encoding.Init
    @Encoding.Copy
    void set(
      final Option<T> opt)
    {
      this.optional = opt;
    }

    @Encoding.Init
    @Encoding.Copy
    void set(
      final T x)
    {
      this.optional = Option.of(x);
    }

However, this results in:

VavrOptionEncoding.Builder.set(): Duplicate builder member name 'set'. Encoding has limitation so that any duplicate method names are not supported, even when allowed by JLS: methods cannot have overloads here. @Encoding.Naming annotation could be used so that actually generated methods might have the same name if they are not conflicting as per JLS overload rules

It's not clear to me what I should be putting in the @Encoding.Naming annotation.

[bowbahdoe]

Without the @Encoding.Copy Annotation you can make another method with a different name than set

import io.vavr.control.Option;
import org.immutables.encode.Encoding;

import java.util.Objects;

@Encoding
class OptionEncoding<T>
{
    @Encoding.Impl
    private Option<T> field = Option.none();

    OptionEncoding()
    {

    }

    @Encoding.Copy
    public Option<T> withOption(
        final Option<T> value)
    {
        return Objects.requireNonNull(value);
    }

    @Encoding.Copy
    public Option<T> with(
        final T value)
    {
        return Option.some(value);
    }

    @Encoding.Builder
    static final class Builder<T>
    {
        private Option<T> optional = Option.none();

        Builder()
        {

        }

        @Encoding.Init
        @Encoding.Copy
        void set(
            final Option<T> opt)
        {
            this.optional = opt;
        }

        // Here
        @Encoding.Init
        void setValue(
            final T opt)
        {
            this.optional = Option.of(opt);
        }

        @Encoding.Build
        Option<T> build()
        {
            return this.optional;
        }
    }
}

That will copy over into an overloaded method in the builder.

[io7m]

Thanks, this should be in 0.6.0.