couchbase / couchbase-lite-android-ce

The community edition of couchbase lite for android
Apache License 2.0
9 stars 1 forks source link

Unable to import ReplicatorType from CBL v2.6.0 when using Kotlin #22

Closed Murilo-Perrone closed 5 years ago

Murilo-Perrone commented 5 years ago

Since v2.0, the ReplicatorType enumeration has been nested in ReplicatorConfiguration class, which made it accessible with the syntax below:

ReplicatorConfiguration.ReplicatorType.PUSH_AND_PULL

However, in v2.6.0 this enum has been moved into a new class named AbstractReplicatorConfiguration, which is not public (it is package private). For my app, this has rendered the ReplicatorType enumeration inaccessible. As a result, I'm unable to configure the replicator type, which prevents me from using v2.6.0. I have tried in Kotlin and same result.

bmeike commented 5 years ago

This seems to work fine in Java. I confirm (and was pretty surprised) that it does not work in Kotlin.

bmeike commented 5 years ago

Tracked in https://issues.couchbase.com/browse/CBL-370

bmeike commented 5 years ago

Unfortunately, this will require a breaking change in our API. It is unlikely to be fixed in the near future.

bmeike commented 5 years ago

@Murilo-Perrone can you confirm that you see this only in Kotlin code?

Murilo-Perrone commented 5 years ago

@bmeike yes, I confirm the issue does not occur for Java client code, just Kotlin, and I have updated the issue topic. Yet no success with Kotlin 1.3.31 nor 1.3.50 (latest)...

Screen Shot 2019-09-18 at 11 45 31

But I did a workaround by creating adding a Java class to my project:

import com.couchbase.lite.ReplicatorConfiguration;

public class ReplicatorTypeHelper {
    public static ReplicatorConfiguration.ReplicatorType getReplicatorTypeFor(Boolean pull, Boolean push) {
        if (pull && push) return ReplicatorConfiguration.ReplicatorType.PUSH_AND_PULL;
        if (pull) return ReplicatorConfiguration.ReplicatorType.PULL;
        if (push) return ReplicatorConfiguration.ReplicatorType.PUSH;
        return null;
    }
}

Interestingly, Kotlin code still recognizes the problem and produces a warning:

Screen Shot 2019-09-18 at 12 09 23 The warning states: Type AbstractReplicatorConfiguration.ReplicatorType! is inacessible in this context due to: public final enum class ReplicatorType : Enum<AbstractReplicatorConfiguration.ReplicatorType!> defined in com.couchbase.lite.AbstractReplicatorConfiguration

I don't know why AbstractReplicatorConfiguration class was created (seems like a code refactor) but this problem with Kotlin client could be reversed by changing this class into public.

bmeike commented 5 years ago

Yes. Our library is not very Kotlin friendly. :-(. It is something that I intend to fix. AbstractReplicatorConfiguration exists to allow us to avoid publishing functionality necessary for internal use into the public API. Moving that type out of its containing class will, unfortunately, change the public API (those enum values will have new names). It is a small change and am pushing for it (because not making it makes CBL all but useless for modern Android apps) but, as I say, we are just not very Kotlin friendly, over here. I expect to build a Kotlin support library and was thinking about a helper, like yours, as a workaround for this issue. If I credit you, can I steal yours?

bmeike commented 5 years ago

I'm going to close this, now. The current, unfortunate answer is: you are right, our lib is not very usable in Kotlin. I am tracking this specific problem in https://issues.couchbase.com/browse/CBL-370 and am committed to addressing Kotlin friendliness in general. In the main time, we just aren't.

Upon reflection, I don't think your workaround is going to help. You will not be able, in Kotlin, to express the type that is returned to you by that helper function. I think you are gonna want something like this:

public final class ReplicatorHelper {
    private ReplicatorHelper() {}

    public enum ReplicationType {PUSH_AND_PULL, PUSH, PULL}

    public static void setReplicatorType(@NonNull ReplicatorConfiguration config, @NonNull ReplicationType type) {
        config.setReplicatorType(
            (type == ReplicationType.PUSH_AND_PULL)
                ? ReplicatorConfiguration.ReplicatorType.PUSH_AND_PULL
                : ((type == ReplicationType.PUSH)
                    ? ReplicatorConfiguration.ReplicatorType.PUSH
                    : ReplicatorConfiguration.ReplicatorType.PULL));
    }

    public static ReplicationType getReplicatorType(@NonNull Replicator repl) {
        switch (repl.getConfig().getReplicatorType()) {
            case PUSH_AND_PULL:
                return ReplicationType.PUSH_AND_PULL;
            case PUSH:
                return ReplicationType.PUSH;
            case PULL:
                return ReplicationType.PULL;
        }
        throw new IllegalArgumentException("huh?");
    }
}
Murilo-Perrone commented 5 years ago

The helper class resolved the problem for me because in Kotlin we don't need to repeat type references over and over. Once the enumerated value is set in a variable, I don't need to reference ReplicatorType anymore.

So AbstractReplicatorConfiguration is package private so that internal stuff from ReplicatorClass is no exposed to CBL API. Fine. But then why has ReplicatorType enumaration moved into the abstract class ? Shouldn't this enum be moved back to the public and concrete ReplicatorConfiguration class ? That would work great for Kotlin and Java, with changes only in internal code, not in the API.

bmeike commented 5 years ago

so:

val replType = getReplicatorTypeFor(true, true)

That's great if it works. As long as Java has generated the synthetic accessors, as long as you don't have to say the type's name (it's like Voldemort...) you should be fine

... and, yeah. The other thing that drives our packaging architecture is that we have 4 products that share code. Code that is common to all of them goes into common base-classes. But really, that's just a reason, not an excuse. It should, as you say, come out of there.