erikdoe / ocmock

Mock objects for Objective-C
http://ocmock.org
Apache License 2.0
2.16k stars 606 forks source link

Rename "wasUsed" to "ocm_wasUsed" #429

Closed dmaclach closed 4 years ago

dmaclach commented 4 years ago

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.

erikdoe commented 4 years ago

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.

dmaclach commented 4 years ago

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 notifications@github.com 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 https://github.com/erikdoe/ocmock/pull/429#issuecomment-639152020, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACOFSIYFZN6SJFXVABMMOTRVAOFBANCNFSM4NNNXW6A .

carllindberg commented 4 years ago

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.

erikdoe commented 4 years ago

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 461c6941537e6bc95d3961ae4b2156b39d684213.