Rename "wasUsed" to "ocm_wasUsed" #429
Conversation
Objects that we were mocking had a wasUsed field that prevented them from being easily mocked using OCMStub, OCMExpect etc. All future methods in recorders should be prefixed with ocm_ to prevent potential conflicts.
|
To be honest, conceptually I get the issue, but I'm not sure about changing one method and stating future methods should have a prefix and then changing one of the existing ones, but not all of them. Of course, I see (now) that |
|
Putting the prefix on methods is what Apple has been recommending (
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/CustomizingExistingClasses/CustomizingExistingClasses.html#//apple_ref/doc/uid/TP40011210-CH6-SW4).
We can't change the other methods without breaking a lot of clients, but I
think it makes sense to require new methods to be as safe as possible.
`didRecordMethod` may be ok, but see #431 where I am suggesting adding
location to all of the recorders.
…On Thu, Jun 4, 2020 at 3:31 PM Erik Doernenburg ***@***.***> wrote:
To be honest, conceptually I get the issue, but I'm not sure about
changing one method and stating future methods should have a prefix and
then changing one of the existing ones, but not all of them. Of course, I
see (now) that wasUsed has a much higher chance of a clash than the other
methods. Maybe the solution to your problem would be to rename wasUsed to
didRecordMethod or something similar, which has an equally low chance to
clash as the other methods on recorder.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#429 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACOFSIYFZN6SJFXVABMMOTRVAOFBANCNFSM4NNNXW6A>
.
|
|
Technically that guidance is adding methods to classes you don't "own", i.e. categories. Granted, these are NSProxy subclasses, and any method on them will prevent the forwardInvocation: stuff from being called, meaning methods of the same names in "real" classes cannot be mocked -- so this is something of a similar situation. I generally agree that the proxy subclasses should not have generically-named methods. On the other hand, I can understand not wanting to use an "ocm_" prefix, but use other method names that somewhat scope things to OCMock internals to make name collisions very unlikely. People can always adjust their own method names to avoid collisions, too, if OCMock's names are specific enough. The "location" method could be "recordingLocation" or "mockLocation" or something else that reads better than having explicit prefixes. That's more a stylistic thing so more Erik's call -- though I would agree that it would be best to be consistent, and either change all of them or none, and don't mix. |
This is an alternative to #429. Note that I removed the setting of the flag in `doesNotRecognizeSelector:`. This seemed superfluous now because the only code that checks the flag first checks whether the invocation threw. Because of that I chose the name didRecord, and not didReceive. The latter would have been more precise if I had kept the setting of the flag in `doesNotRecognizeSelector:`. I am aware that using a prefix would have decreased chances of a clash even further, but I think this is good enough and it's more consistent with the rest of the codebase for now.
|
So, I ended up with not using a prefix but a method name that is much less likely to cause a clash. I've left more detail in the commit message on 461c694. |
Objects that we were mocking had a
wasUsedfield that prevented them from being easily mocked usingOCMStub,OCMExpectetc.All future methods in recorders should be prefixed with
ocm_to prevent potential conflicts.