Persistable protocol fix

[max-pfeiffer]

Dear maintainer, looking at https://github.com/RxSwiftCommunity/RxCoreData/pull/23 and the RxDatasources implementation of cell updates, the Persistable protocol needs some more changes. Problem is it interferes with IdentifiableType protocol of RxDataSources.

Will provide updates for the examples as well after this is merged.

[max-pfeiffer]

Some more explanation: IdentifiableType is using property 'identity' as well. But this is of Hashable type to make it dynamic. So you can use i. e. Int or String as identifier. Coming from a SQLite Database identifiers are usually Int64.

In Persistable protocol the identity property is of type String, which is a very narrow use case. Plus you don't need necessarily a default implemetation for 'func predicate() -> NSPredicate', because this again is narrowing down your use case.

https://github.com/RxSwiftCommunity/RxCoreData/pull/23 is a good approach, but needs a bit more fine tuning.

[max-pfeiffer]

@bobgodwinx , is there a reason why this isn't merged yet? I saw you published 0.5.0.

[bobgodwinx]

@max-pfeiffer I just wanted to carry out some test on this. Don't worry it will be merged asap that is done. Thanks

[freak4pc]

Hey @max-pfeiffer and @bobgodwinx !

I want to start off by thanking you for your contribution, and I agree with you there's definitely some sort of collision going on here. Unfortunately, I don't think this is really a "solution" - merely removing the entire protocol just to not cause a conflict isn't a solid enough solution in my opinion as both frameworks actually need it.

My two thoughts moving forward would be:

1. Leave as is: Given an "identity" represents something that is immutable and never changes, shouldn't it actually be perfectly fine sharing this between RxDataSources and RxCoreData ?

OR

2. Rename: Rename identity to persistableIdentity in RxCoreData so its specific to this framework, and hard-deprecate identity conformance. This would need to be part of a minor/major release IMO.

Open to any thoughts and discussion around this, and again - thanks for chiming in!

Shai.

[bobgodwinx]

@max-pfeiffer You can go for the second option persistableIdentity which sounds like a better compromise. Secondly use git rebase --interactive to drop all other unnecessary commits thanks.

[max-pfeiffer]

@bobgodwinx and @freak4pc : Sorry guys, I don't see why with https://github.com/RxSwiftCommunity/RxCoreData/pull/23 (which was already merged) this property is actually needed any more. With https://github.com/RxSwiftCommunity/RxCoreData/pull/23/commits/c9f4257ede3152cb19be5c99b592b6c084081111 the function "func predicate() -> NSPredicate" was added to the Persistable protocol, which is used to determine the objects for inserts and updates. My point is not to remove the Persistable protocol, but to make it more flexible to cover more use cases.

This is great because you could have full flexibility for any use case. Please see my example class I am using with my fork down below for a use case. In my opinion this is making everything much more flexible and simpler.

Which narrows down the options quite a bit is the identity property used by the default implementation of "func predicate() -> NSPredicate".

I agree to go for persistableIdentity for the sake of backwards compatibility. Looking at the data type String of persistableIdentity, shouldn't we change it to something more flexible like Any? This shouldn't break any existing code.

import Foundation
import RxCoreData
import RxDataSources

struct PersistableRestaurant {
    var id: Int64
    var imageURL: String?
    var lastModified: NSDate?
    var name: String?
    var position: Int64
    var resort: Int64
    var htmlURL: String?
}

extension PersistableRestaurant: Persistable {

    typealias T = Restaurant

    static var entityName: String {
        return "Restaurant"
    }

    static var primaryAttributeName: String {
        return "id"
    }

    init(entity: T) {
        id = entity.id
        imageURL = entity.imageURL
        lastModified = entity.lastModified
        name = entity.name
        position = entity.position
        resort = entity.resort
        htmlURL = entity.htmlURL
    }

    func update(_ entity: T) {
        entity.id = id
        entity.imageURL = imageURL
        entity.lastModified = lastModified
        entity.name = name
        entity.position = position
        entity.resort = resort
        entity.htmlURL = htmlURL
    }

    func predicate() -> NSPredicate {
        let predicate = NSPredicate(format: "%K = %@", PersistableRestaurant.primaryAttributeName, NSNumber(value:id))
        return predicate
    }
}

extension PersistableRestaurant: Equatable {
    static func ==(lhs: PersistableRestaurant, rhs: PersistableRestaurant) -> Bool {
        return lhs.imageURL == rhs.imageURL
            && lhs.name == rhs.name
            && lhs.position == rhs.position
            && lhs.resort == rhs.resort
            && lhs.htmlURL == rhs.htmlURL
    }
}

extension PersistableRestaurant: IdentifiableType {
    typealias Identity = Int64

    var identity: Identity {
        return id
    }
}

[freak4pc]

Thanks for your feedback.

The reason for leaving an identity-like thing is that people don't have to manually implement the predicate() method, since most people do not need that level of granularity. I strongly disagree on the fact it makes identity obsolete.

I don't think Any is qualified as an identity. Identity is an immutable identifier. Please see the difference between Identity and Equatable discussed over in RxDataSources as it might help you clear things up.

Plus - Adding PATs for no good reason is just painful. PATs, besides for the performance implication, make things very hard to extend and pass on without falling into tricks such as type erasure. If there would be any significant values to support a Generic type for an identity I would gladly dig more into this but I just do not see any value. You can easily create an identity for any entity in the form of a String. I don't see how a Concrete type can constitute as a unique identity for the majority of users. (I know this is used in RxDataSources but it is generally not used for non-String types)

If you have a specific use-case where this is extremely important, we can definitely discuss.

[max-pfeiffer]

@freak4pc : Thanks for your opinion on this. Yes, lets not use PATs, just painful. I am fine with a method override of "func predicate() -> NSPredicate" which offers the needed flexibility.

@bobgodwinx : So I will go with persistableIdentity. I would do the changes and merge in the latest head of the master branch resolving any conflicts. This should work as well or do I get anything wrong? Sorry, not beeing a fan of rebase and a big contributor in the past.

[freak4pc]

@max-pfeiffer Just make the changes, I'll take care of the cleanup :) I'll review and clean after you make the changes.

[max-pfeiffer]

@bobgodwinx and @freak4pc : I finished my work. I also fixed the example. I had to merge in the 0.5.0 master branch and tweaked the Podfile to install the pods for the example.

[max-pfeiffer]

@freak4pc : I've got your point. I just ran some test on my side: yes, it's possible to work with the Persistable protocol with the identity property using type conversions.

Let's skip the renaming to persistableIdentity. In practice this is not leading anywhere for now.

I hope you agree that it is kind of an awkward construction to define a protocol with identical properties of another protocol in a different indipendent library. Shouldn't this be decoupled? What if changes to IdentifiableType in RxDatasources occur?

But in the moment I don't see any proper solution for it without using PAT.

So, I will revert the unnecessary changes. At least I found an error in Example/Podfile . :-) I will also add some comments in the example to point out the use of type conversions explicitly which might help some users in the future.

[freak4pc]

Thanks @max-pfeiffer I don't think the comment adds value because in that specific sample it uses a typealias since IdentifiableType actually uses a PAT.

It is a bit annoying that protocols can define something called "identity" but that identity can be shared via several protocols (causing a conflict), but in this specific case - an identity is the same identity across all items.

I don't think this PR "As is" is currently mergeable (there are almost 10 commits, mostly reverts). If you have any issues you found in the example project or Podfile, please submit a separate PR so we can get a clean review in :)

Thanks for your help. Shai.