pivotal-legacy / PivotalCoreKit

Shared library and test code for iOS and macOS projects
http://pivotallabs.com
Other
168 stars 85 forks source link

Move PCKMethodRedirector to Foundation+SpecHelper and make it public #178

Closed younata closed 7 years ago

tjarratt commented 7 years ago

VERY Interesting idea!

What's the use case? I can see this being helpful (I've certainly wanted to use it on projects...), but there's also probably the counter argument of "Method redirection and other runtime shenanigans are dangerous, and best applied sparingly. Making it easy is a double-edged sword".

Also, did you know there is already a spec for this?

The spec you added seems pretty small, but I can't tell if it adds anything new. Perhaps we should just consolidate them under a unified name?

tjarratt commented 7 years ago

OH wow this is from February.

Where did the time go?

That was such a simpler time...

younata commented 7 years ago

Of course there's a not-at-all similarly named spec for this. :/

Use case is when I want to make my own custom stubs. rNews makes use of this behavior in a few places. (WKWebView stubs, UIViewController showController/showDetailController stubs, UITraitCollection stubs).

Basically, places where I wanted to make a stub, but didn't care to take the time to contribute the stub to PCK.

Relatedly, I should take some time and make pull requests for those...

younata commented 7 years ago

Huh. There is apparently also an NSObject+MethodDecorator category that... I don't really see the point of having.

Might open up another pull request to delete it, unless you have other thoughts?

@tjarratt @akitchen

akitchen commented 7 years ago

Maybe start by removing that compilation unit and running all the specs?

I believe it is there to allow renaming of existing methods in order to keep them available via a category interface, I.e -[SomeClass foo_orig:]

On Nov 22, 2016, at 14:47, Rachel Brindle notifications@github.com wrote:

Huh. There is apparently also an NSObject+MethodDecorator category that... I don't really see the point of having.

Might open up another pull request to delete it, unless you have other thoughts?

@tjarratt @akitchen

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

tjarratt commented 7 years ago

+decorateMethod:(NSString *)methodName with:(NSString *)decoratedName does appear to be entirely unused (within PCK). Seems like deleting it is eminently reasonable, from the perspective of maintaining the internals of PCK.

However, I think (think?) it is technically part of the public API, since it's present in what appears to be a public header.

Might be worth keeping it around for this PR and then tackle it in a separate PR.

akitchen commented 7 years ago

I think this may have been left over from the old NSURLConnection spec helper code. I agree it's probably safe to delete if there are feelings that it's no longer needed.

On Nov 22, 2016, at 22:22, Tim Jarratt notifications@github.com wrote:

+decorateMethod:(NSString )methodName with:(NSString )decoratedName does appear to be entirely unused (within PCK). Seems like deleting it is eminently reasonable, from the perspective of maintaining the internals of PCK.

However, I think (think?) it is technically part of the public API, since it's present in what appears to be a public header.

Might be worth keeping it around for this PR and then tackle it in a separate PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.