Skip to content
This repository has been archived by the owner on Mar 24, 2022. It is now read-only.

Convert getter-like methods into readonly properties #181

Merged
merged 1 commit into from
Feb 8, 2016

Conversation

briancroom
Copy link
Contributor

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.


@implementation PCKMessageCapturer
@implementation PCKMessageCapturer {
NSMutableArray *_sent_messages;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are modernizing, why move this to an ivar?

@akitchen
Copy link
Contributor

akitchen commented Feb 4, 2016

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
Copy link
Contributor Author

@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 😬)

@akitchen
Copy link
Contributor

akitchen commented Feb 6, 2016

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
#181 (comment)
.

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

@briancroom
Copy link
Contributor Author

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 added a commit that referenced this pull request Feb 8, 2016
Convert getter-like methods into readonly properties
@akitchen akitchen merged commit 54d1f6d into pivotal-legacy:master Feb 8, 2016
@akitchen
Copy link
Contributor

akitchen commented Feb 8, 2016

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

@briancroom briancroom deleted the prefer-properties branch February 9, 2016 16:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants