uber / mockolo

Efficient Mock Generator for Swift
Apache License 2.0
804 stars 85 forks source link

Add ability to override generated mock name #214

Closed ileitch closed 1 year ago

ileitch commented 1 year ago

When creating mocks from protocols for external types, the generated mock may have an undesirable name:

/// @mockable
public protocol UINavigationControllerProtocol { ... }
public class UINavigationControllerProtocolMock: UINavigationControllerProtocol { ... }

This change adds the ability to override the generated mock name:

/// @mockable(override: name = UINavigationController)
public protocol UINavigationControllerProtocol { ... }
public class UINavigationControllerMock: UINavigationControllerProtocol { ... }
CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

sidepelican commented 1 year ago

Thanks for the PR. And sorry, I did not understand why you wanted this feature. If the type name is important, Swift has typealias feature. If the type name conflicts to other type, you can split the module.

I would not want to add more features than necessary because of the maintenance.🙇‍♂️

ileitch commented 1 year ago

It's common in tests to want to mock types that we don't have control over, e.g UINavigationController, FileManager, NotificationCenter, etc. In order to mock them, we first need to write a custom protocol e.g UINavigationControllerProtocol to use in place of the concrete type at the injection site.

Mockolo's default naming convention of appending "Mock" results in a weird name such as UINavigationControllerProtocolMock. This PR allows us to override the type name and generate a more natural name for the mock.

You're right that typealias is one option, but that'd require us to declare a typealias for all external types we want to mock, which would make Mockolo inconvenient to use. A typealias would also result in two public types, which will surely cause some confusion.

fummicc1 commented 1 year ago

Thank you for creating PR and sorry to check it. 🙇‍♂️

from my side, this feature is very nice because we can make mockolo more convenient than before, but it also increases our maintainance cost. for current mockolo, I also guess it is not good idea to introduce arbitrary feature that does not fix something broken. but such a improvement like this PR should be welcome in my understanding. so I think we can start reviewing for this PR. What do you think of it?

Besides that, to prevent same concern in the future, how about creating a PR template and each PR should be related to an issue which is already discussed about the need ?

sidepelican commented 1 year ago

Hmm, I kind of understand. UINavigationControllerProtocolMock is bit a long. But this is not logically reasonable. I think it is a matter of feeling.

As for the shortcomings of typealias, I think you are right. I was thinking some metaprograming usecase, not for human.

Also, this option is unfriendly to newcomers of the project. For those who already know mockolo but are new to the project's code, I think it would cause confusion if this option were used.

I am not positive about this feature, but this is so simple that there are less problems of a maintenance. Also, this is not mandatory for all users. If some people want to use it, I think I could merge this.

sidepelican commented 1 year ago

Thank you !