Exclude weak properties from equality checks etc. #204

Closed
robb opened this Issue Dec 22, 2013 · 20 comments

Comments

Projects
None yet
2 participants
Owner

robb commented Dec 22, 2013

Previously, on #202:

@jspahrsummers:

I've considered it in the past, but I've avoided it because of worries about being too magical.

I see that, but it seems like it would lead to a better out of the box experience.

I've looked at some of the previous issues regarding the infinite loops and in #88, #190 and #202 weak was used correctly for the cyclic back-reference.

Owner

robb commented Dec 22, 2013

gah, I realized that the point is moot since we don't have implicit mapping anymore…

robb closed this Dec 22, 2013

Owner

jspahrsummers commented Dec 22, 2013

True, but this is still applicable to dictionaryValue, -isEqual:, -description, etc.

jspahrsummers reopened this Dec 22, 2013

Owner

robb commented Dec 22, 2013

Oh, yeah, jeez. I guess I should ☕️ before I 💻

Owner

robb commented Dec 22, 2013

Anyhow, I guess we'd still want to include weak properties -copy and maybe -dictionaryValue, too?

Owner

jspahrsummers commented Dec 23, 2013

That may have actually been one reason why I originally stopped trying to special case weak properties: it doesn't make sense to remove them entirely from +propertyKeys, only certain behaviors.

That, in turn, makes it more difficult to change those behaviors in subclasses, since there are now multiple places to override. It sounds like we really want a more generic version of +encodingBehaviorsByPropertyKey.

Owner

robb commented Dec 23, 2013

That, in turn, makes it more difficult to change those behaviors in subclasses, since there are now multiple places to override.

I agree, but I think all things being equal, adding weak properties in a subclass is simpler than removing them, where you'd have to re-implement the affected methods from scratch without calling super.

It sounds like we really want a more generic version of +encodingBehaviorsByPropertyKey.

Something along the lines of

typedef NS_OPTIONS(NSUInteger, MTLPropertyBehavior) {
    MTLPropertyBehaviorEquality, // and hash
    MTLPropertyBehaviorCopying, // and dictionary value 
    MTLPropertyBehaviorDescription
};

@interface MTLModel

// Returns MTLPropertyBehaviorCopying for weak properties and 
// MTLPropertyBehaviorEquality | MTLPropertyBehaviorCopying | MTLPropertyBehaviorDescription otherwise
+ (MTLPropertyBehavior)behaviorForPropertyWithKey:(NSString *)propertyKey;

@end

?

Owner

jspahrsummers commented Dec 23, 2013

Yeah, maybe something like that. I'm not a big fan of the masking, but I can't think of a hierarchical way to structure the options otherwise.

Owner

robb commented Dec 23, 2013

Yeah, me neither. I could see a configuration struct or an NSSet of tokens of some kind but this seems to be more idiomatic.

Btw, does libextobjc have a Java-Style enum? If not, maybe I'll try to hack something together for fun.

Owner

robb commented Dec 23, 2013

What do you think about treating -copy-ability and -dictionaryValue-ability the same? I like the idea that an MTLModel's -dictionaryValue is just its data without any pesky behavior…

Owner

jspahrsummers commented Dec 24, 2013

What do you think about treating -copy-ability and -dictionaryValue-ability the same?

👍 Makes sense to me. Copying should be equivalent to instantiating a new model with the same dictionary value.

Also, it occurred to me that the method should be +behaviorsByPropertyKey or something instead, to make it easy to specify behaviors for a set of properties declaratively.

Btw, does libextobjc have a Java-Style enum?

Nope, but I have Opinions™.

Owner

robb commented Dec 24, 2013

Something along the lines of

+ (NSDictionary *)behaviorsByPropertyKey {
    return @{
        @"foo": @(MTLPropertyBehaviorEquality | MTLPropertyBehaviorCopying),
        @"bar": @(MTLPropertyBehaviorDescription),
        @"baz": @(MTLPropertyBehaviorEquality | MTLPropertyBehaviorCopying | MTLPropertyBehaviorDescription)
    };
}

?

I agree that this makes it easier to declare the behavior on a key-by-key basis yet +behaviorForPropertyWithKey: seems simpler and wouldn't preclude subclasses from rolling their own declarative version if need be.

Owner

jspahrsummers commented Dec 26, 2013

How about this enum instead:

typedef enum : NSUInteger {
    // The property is not included in -description, -hash, or anything else.
    // Assumably, it can be regenerated on demand.
    MTLPropertyStorageNone,

    // The property is included for one-off operations, like copying,
    // -dictionaryValue, and -description. It may disappear at any time.
    MTLPropertyStorageTransitory,

    // The property is included in serialization (like `NSCoding`) and equality,
    // since it can be expected to stick around.
    MTLPropertyStoragePermanent,
} MTLPropertyStorage;

Then we don't have to deal with masking or nonsensical combinations of the above flags. If we do this, though, -description shouldn't be so deep as to cause cycles (since weak properties will still be included by default).

I agree that this makes it easier to declare the behavior on a key-by-key basis yet +behaviorForPropertyWithKey: seems simpler and wouldn't preclude subclasses from rolling their own declarative version if need be.

👍 Fair enough.

Owner

robb commented Jan 1, 2014

Makes sense, maybe we can find names that are less intimidating? It seems unexpected that a node's weak parent property is considered transitory even though it sticks around.

Owner

jspahrsummers commented Jan 1, 2014

That's not strictly true. The child can continue to exist after the last strong reference to its parent disappears. That's the sort of thing I mean by "transitory."

Owner

robb commented Jan 1, 2014

Fair enough 👌

Owner

robb commented Jan 2, 2014

If we do this, though, -description shouldn't be so deep as to cause cycles (since weak properties will still be included by default).

I think having deep descriptions, such as printing the contents for array or dictionary properties, is desirable though.

Owner

jspahrsummers commented Jan 2, 2014

My thinking was that we could recurse collections deeply, and pretty-print certain value types (like NSValue), but skip a full description of nested MTLModels and other complex types.

Owner

robb commented Jan 2, 2014

That sounds somewhat complicated but doable.

That being said, what if only MTLPropertyStoragePermanent properties were part of description? Seems that the ones that can't disappear at any time describe an object better. That would mean that -[MTLModel description] would be more than just [[self dictionaryRepresentation] description], though.

Owner

jspahrsummers commented Jan 3, 2014

Maybe that is the best choice. It'd be simpler, and we can always change it later (since this isn't part of an API contract).

Owner

robb commented Mar 14, 2014

Closed by #210

robb closed this Mar 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment