Follow up for "Protected builder and copy methods to hide internal flags"

[mcliedtke]

Hello,
Really appreciate you taking a look at #690 and I just had a couple follow ups since I was only just recently able to pull down 2.6.1. I was hoping you might be able to look into the following which may polish the feature off quite nicely.

Here's my convoluted example class I will refer to:

@Value.Immutable
@Value.Style(
        overshadowImplementation=true,
        alwaysPublicInitializers=false)
public abstract class InternalFlagClass implements WithInternalFlagClass {
    
    @Value.Default
    protected boolean internalFlag() {
        return false;
    }

    public abstract String someField();
    
    @Value.Check
    protected InternalFlagClass constructionCleanup() {
        if(this.internalFlag()) {
            return this;
        }
        String sanitizedFieldvalue = sanitizeField(this.someField());
        return ImmutableInternalFlagClass.builder()
                .internalFlag(true)
                .someField(sanitizedFieldvalue)
                .build();
    }

    private String sanitizeField(String someField) {
        //some santization that modifies the string
        //...
    }
    
    public static class Builder extends ImmutableInternalFlagClass.Builder {}
    public static Builder builder() {
        return new Builder();
    }
}

With 2.6.1, I can set alwaysPublicInitializers=false and this will maintain protected for the builder method for internalFlag. There are two areas that would be nice to continue to "hide" the visibility of the field:

1. It would also be nice if this applied to the withInternalFlag copy method, both in the generated ImmutableInternalFlagClass class and WithInternalFlagClass interface. In the class, I would expect the method to be protected (or maybe just removed) and in the interface, I would expect the method to be removed.

2. In the generated builder, ImmutableInternalFlagClass.Builder, it would be helpful to be able to omit this field from the from copy method.

public final class ImmutableInternalFlagClass extends InternalFlagClass {
  ...
  public final ImmutableInternalFlagClass withInternalFlag(boolean value) { //<-- should be protected
    if (this.internalFlag == value) return this;
    return validate(new ImmutableInternalFlagClass(value, this.someField));
  }
  ...
  public static class Builder {
    ...
    public final InternalFlagClass.Builder from(InternalFlagClass instance) {
      Objects.requireNonNull(instance, "instance");
      internalFlag(instance.internalFlag()); //<-- Allow this to be omitted.
      someField(instance.someField());
      return (InternalFlagClass.Builder) this;
    }
    ...
  }
}

public interface WithInternalFlagClass {
  InternalFlagClass withInternalFlag(boolean value); //<-- should be removed
  InternalFlagClass withSomeField(String value);
}

[elucash]

Thank for reporting this. Would agree with most of the points, but feel that omitting from from copy method would need to be justified by some other concept or trick. i.e. having marked as protected should not imply that it will not be copied, there should be something else. Suggestions welcome

[mcliedtke]

Yeah I think it makes sense to have a different mechanism trigger the from behavior.

This seems somewhat tied to the @Value.Default annotation, so one option could be to add a parameter there to control this. Naming seems a bit difficult depending on how you want to scope/phrase the feature, one possibility could be:

• builderAlwaysInitializedWithDefaultValue (true/false, default to false): out of context, this might be confusing but seems to get at the core feature with the idea being that from creates a new builder and you can enable that new builder to start with the copied value or the default value. true to use the default value when builder is initialized from from, false to maintain current logic.

If it makes sense to extend this kind of behavior to even non-default fields (i.e. maybe I always want the user to specify a value at construction, even copying from another isntance), then perhaps a new annotation would make more sense?