search

pivotal-legacy / PivotalCoreKit

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

Convert getter-like methods into readonly properties #181

Closed briancroom closed 8 years ago

briancroom commented 8 years ago

This improves the Swift bridging. The methods get bridged into Swift as functions, which need to be invoked with () which feels un-idomatic when they are intended to effectively be getters.

This is especially problematic in cases like tableRowAction.handler() which (previously) would return the handler closure, but not actually invoke it, despite appearances.

Thanks to @pivotal-rebecca-chin for first making me aware of this issue and tracking down what was going on.

akitchen commented 8 years ago

Looks great, except that I don't understand the introduction of the ivar. does this help somehow, or was it a style choice? If the latter, I would prefer we not do that here.

briancroom commented 8 years ago

@akitchen ah yeah, I was a little surprised that it was necessary as well. The problem is described here a little. Basically the compiler synthesizes the ivar based on the type declared in the @interface, not the re-declared type in the class extension. It would be possible to use a mutable private property as the backing storage and override the public property's getter to refer to it, but that didn't seem worthwhile in this case.

(Edit: I'll also mention that I now recall having repeatedly forgotten about this quirk of the compiler, and being a little annoyed each time when I've tried to use this pattern in the past :grimacing:)

akitchen commented 8 years ago

Thanks for the explanation! It would be nice if a test target would fail to compile (or fail while running) if someone refactored that. On Thu, Feb 4, 2016 at 23:11 Brian Croom notifications@github.com wrote:

@akitchen https://github.com/akitchen ah yeah, I was a little surprised that it was necessary as well. The problem is described here http://stackoverflow.com/questions/21109083/how-to-make-an-immutable-readonly-property-in-the-header-file-h-a-mutable-re a little. Basically the compiler synthesizes the ivar based on the type declared in the @interface, not the re-declared type in the class extension. It would be possible to use a mutable private property as the backing storage and override the public property's getter to refer to it, but that didn't seem worthwhile in this case.

— Reply to this email directly or view it on GitHub https://github.com/pivotal/PivotalCoreKit/pull/181#issuecomment-180231771 .

Andrew Kitchen Associate Director, Pivotal Labs o +1.415.777.4868 x344 m +1.415.501.0085

briancroom commented 8 years ago

Well, it did fail to compile until I removed the property from the class extension and added the explicit ivar instead. The reason it worked previously was that the public interface had a simple method rather than a property.

It would be nice to have some tests requiring these to remain property declarations, but I'm not sure how to do that without adding one or more Swift spec files which presents some challenges, in particular with CI and Xcode versions. Do you think it's worth it?

akitchen commented 8 years ago

Works for me. Thanks @pivotal-rebecca-chin & @briancroom !