Conversation
| if([self didRecordInvocation] && [self currentlyRecordingInMacro]) | ||
| { | ||
| [NSException raise:NSInvalidArgumentException | ||
| format:@"Recorder attempting to record `%s` but recorder has already recorded a stub for: `%@`. " | ||
| @"Are there multiple invocations on mocks in a single macro?", | ||
| sel_getName(aSelector), [self description]]; | ||
| } |
There was a problem hiding this comment.
Why not move this block into forwardInvocation:? There's another assert there anyway, and I cannot see a benefit for detecting this case here.
| { | ||
| returnValue = mockObject; | ||
| } | ||
| else if ([anInvocation methodIsInCreateFamily]) |
There was a problem hiding this comment.
Not sure this needs to be in an else branch. Somehow I find the implementation confusing. First we set a "default" return value outside the if statement, then we overwrite it in the if statement, or, alternatively, we manipulate the original value in an else-if. I get why the inCreateFamily check is there, and it's a good additional fix to the change in the PR, but I think I'd restructure this when merging. (No action needed on your part.)
| - (BOOL)currentlyRecordingInMacro | ||
| { | ||
| return [[OCMMacroState globalState] recorder] == self; | ||
| } |
There was a problem hiding this comment.
If we move the check from methodSignatureForSelector: to forwardInvocation: then the code in this method is only needed once. I find the name confusing and can't think of a better name. In any case, I think any name wouldn't add much to explain what's happening and given this method is only needed once I'd inline it.
2 participants
This is intended as a fix for #478