-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Handle Error Pointers according to Cocoa Convention #3653
Conversation
Current coverage is 87.44% (diff: 100%)@@ master #3653 diff @@
==========================================
Files 44 45 +1
Lines 6067 6198 +131
Methods 1079 1096 +17
Messages 0 0
Branches 407 412 +5
==========================================
+ Hits 5256 5420 +164
+ Misses 808 775 -33
Partials 3 3
|
I think you missed the point. The return value of the method call needs to be tested before looking at the error. This is what is done in the sample code from Apple you provided and this is not what is done in the AFNetworking code.
|
I don’t understand why this PR was closed, @tclementdev point is perfectly valid. From Cocoa's Error Handling Programming Guide
|
Sorry misread the original intent! Reopened here. Has a full audit of the codebase been done for this particular issue? |
Thank you for re-opening this PR. I just pushed some additionnal fixes concerning the same issue in other system API calls. I could not find any other system API misuse, however it should be noted that the AFNetworking APIs themselves seem to be quite oblivious of this convention, with code seemingly returning non-nil objects while an error occurred internally. There is probably some amount of work to audit and modify the AFNetworking code itself to clarify and address this (this can be done in a separate PR). |
Can you rebase this with master to get Xcode 8 coverage? |
…s (this is the correct way to handle errors as described in Apple's documentation).
Done. |
You can see the codecov diff here. Verifying those nil's should help prevent regressions in the future. After that, should be good to merge! |
return nil; | ||
} | ||
|
||
[mutableRequest setHTTPBody:jsonData]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test verifying the nil behavior here?
|
||
if (!plistData) { | ||
return nil; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a test to verify nil behavior here. Just so we can catch regressions in the future.
if (error) { | ||
*error = AFErrorWithUnderlyingError(serializationError, *error); | ||
} | ||
return nil; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test verifying nil
if (error) { | ||
*error = AFErrorWithUnderlyingError(serializationError, *error); | ||
} | ||
return nil; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test verifying nil
if (data) { | ||
responseObject = [NSPropertyListSerialization propertyListWithData:data options:self.readOptions format:NULL error:&serializationError]; | ||
if (!data) { | ||
return nil; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test verifying nil
Am I good? |
🍻 thanks! |
This is a Cocoa convention.