Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OptionalProperty based on PropertyType #448

Open
ljacqu opened this issue Oct 8, 2024 · 0 comments
Open

OptionalProperty based on PropertyType #448

ljacqu opened this issue Oct 8, 2024 · 0 comments
Milestone

Comments

@ljacqu
Copy link
Member

ljacqu commented Oct 8, 2024

Currently, OptionalProperty has the following constructors:

    public OptionalProperty(@NotNull Property<T> baseProperty) {
        this.baseProperty = baseProperty;
        this.defaultValue = Optional.empty();
    }

    public OptionalProperty(@NotNull Property<T> baseProperty, @NotNull T defaultValue) {
        this.baseProperty = baseProperty;
        this.defaultValue = Optional.of(defaultValue);
    }

In ConfigMe 2.0, we want to make property types more prominent because they can be combined more easily. Thus, the following constructor would be desirable:

    public OptionalProperty(@NotNull String path, @NotNull PropertyType<T> propertyType,
                            @NotNull T defaultValue) {
        this.baseProperty = new TypeBasedProperty<>(path, propertyType, defaultValue);
        this.defaultValue = Optional.of(defaultValue);
    }

^ This constructor, however, forces that a default value be provided. What if the user wants Optional.empty to be the default? We need a default value in order to construct the TypeBasedProperty.

To do

Goal of this issue is to revise the implementation of OptionalProperty:

  • Change OptionalProperty to have a constructor like above, as well as a constructor without the defaultValue param
    • In order to achieve this, the field baseProperty must be replaced by a PropertyType field and a nullable defaultValue field
  • Adapt usages in PropertyInitializer to no longer have the fake default value (very nice change!)
  • In order to maintain compatibility with a "base Property", introduce some way to still be able to create an OptionalProperty based on another Property<?>. We cannot assume that it has a PropertyType that we can extract from it.
    • Probably introduce some extension of OptionalProperty that overrides all the necessary behavior. This will be ugly (because some fields in the parent will be unused) but this will maintain backwards compatibility for those who wish.
    • With some static method on OptionalProperty we could even hide the fact that an extension is at play.
  • In PropertyInitializer, optionalListProperty and optionalSetProperty return an optional for string collections. This is not clear from the name and should be revised. It would be better to add a PropertyType parameter to these methods so that the type of set/list can be specified.
    • Varargs could be added to define the default value, if desired. This does not break current behavior, since 0 entries can still be provided for a varargs. In that case, assume empty Optional as default value
@ljacqu ljacqu added this to the 2.0.0 milestone Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant