Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

Do not log a warning when observing an already observed object key path #84

Merged
merged 1 commit into from
Mar 17, 2016
Merged

Conversation

siyusong
Copy link
Contributor

As mentioned in the documentation, observing an already observed object key path using - observe:keyPaths:... methods should "result in no operation". However, the current implementation logs a warning/info in that case. Since "observing an already observed object key path" is not a misuse of the API, I think the warning is unnecessary. I am relying on the aforementioned behavior in one of my project and found the NSLog messages a little bit annoying.

This is also the only occurrence of NSLog in the whole project.

@nlutsenko
Copy link
Contributor

Do you think we should do something like NSAssert does where the statement is logged only when debugging, but never in release? I am ok merging this as is, to be honest, but it might be helpful to eliminate unwanted behavior...

@nlutsenko
Copy link
Contributor

For example:

#ifdef NS_BLOCK_ASSERTIONS
NSLog(...);
#endif

@siyusong
Copy link
Contributor Author

Just my two cents, but I don't think it's very easy to tell whether the user is unintentionally observing the same object multiple times ("misusing" the API), or if this is actually what they want. From a pure API contract perspective, it is not a misuse of the API since the documentation explicitly allows it, and we should not "penalize" the user who is using the API in a way that is allowed.

@nlutsenko
Copy link
Contributor

Yup, this is valid concern. And taking into account that we have documentation around this method that we are allowing double-observation and it's a no-op at that point - I fully agree with the change. Merging now.

nlutsenko added a commit that referenced this pull request Mar 17, 2016
Do not log a warning when observing an already observed object key path
@nlutsenko nlutsenko merged commit ccf9996 into facebookarchive:master Mar 17, 2016
@nlutsenko nlutsenko self-assigned this Mar 17, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants