Closed jvisenti closed 10 years ago
@jvisenti Please update the tests as well, they are failing now.
@jvisenti Aside from the broken tests and question about keyPath
this looks good. I wonder if there is a way to keep the old block signatures and just deprecate them? Probably not since ObjC doesn't allow method overloading with different param types. The implication is that we have to go to version 3.0 now since this won't be backward compatible (unless we fudge semantic versioning, but that defeats the purpose).
@ndonald2 I don't think there's a way to keep the old blocks, but it's unfortunate to have to go to a new version number for such a small change. We could perhaps put off merging this until there are more major changes?
Why not make a new method that has a slightly different name and takes a RZKVOBlockWithObjectAndKeyPath
or whatever, and deprecate the old one?
@ZevEisenberg Because it really sucks making a slightly different signature just to indicate that the block signature has changed a tiny bit when the method does the exact same thing. Henceforth all calls to that method would use the fudged signature and once the deprecated one is totally gone, the fudged one will be fudged for no reason. On the other hand, if it's too slight of a change, the semantic difference won't be clear.
@ZevEisenberg Not to mention then the internals would have to handle two block types. It's a shame we can't treat blocks as true objects and use reflection/polymorphism with them.
Actually, changing it slightly might not be so bad:
typedef void (^RZKVOBlock)(NSDictionary *change); // Not sure if you can mark a typedef as deprecated
typedef void (^RZKVOChangeBlock)(id object, NSDictionary *change);
- (void)rz_addObserver:(NSObject *)observer forKeyPath:(NSString *)keyPath options:(NSKeyValueObservingOptions)options withBlock:(RZKVOBlock)block __attribute__((deprecated("blah blah")));
- (void)rz_addObserver:(NSObject *)observer forKeyPath:(NSString *)keyPath options:(NSKeyValueObservingOptions)options withChangeBlock:(RZKVOChangeBlock)block;
@jvisenti What's the status on this PR? Did you backtrack on including the keypath?
@jvisenti Also this should be against develop
. Let me know what the status is and I can give it another review.
Changed to RZKVOBlock to include object and keyPath, like the observeValue method does via ordinary KVO.