search

agilebits / onepassword-app-extension

1Password Extension for iOS Apps
2.58k stars 311 forks source link

Add typedefs for completion blocks #339

Closed timothyekl closed 8 years ago

timothyekl commented 8 years ago

Apple's documentation for declaring and creating blocks advocates for naming block types when they are used in multiple places. The current 1Password extension API "spells out" entire block signatures at each use, despite having only three distinct completion block types.

Including typedefs for these three block types would significantly reduce line noise across seven method declarations in OnePasswordExtension.h, as well as several more in the corresponding implementation file.

This branch declares these three types in OnePasswordExtension.h, each with the name pattern OnePassword<Kind>CompletionBlock. "Kind" is:

All "spelled out" block types in method signatures (in both the header and implementation files) are replaced with equivalent typedef'd names. Since several methods exist in the implementation that aren't exposed in the header, and the assume-nonnull range does not extend throughout the implementation file, explicitly mark blocks in such methods nonnull (matching other arguments).

This change does slightly increase the surface area of symbols needing prefixing for developers intending to redistribute this source as part of a framework. I think this is an acceptable tradeoff, but am happy to hear out concerns in that direction. (The branch also adjusts the comment warning about prefixing the class to also mention the new typedefs.)

radazzouz commented 8 years ago

Thanks so much for contributing @lithium3141! 👍

At first glance, the proposed changes look great, except for the couple of renaming issue (see the in-line comments).

I just labeled the PR with under consideration and in review, as we need more time to properly test things out and to make sure that this is indeed something we want.

I will keep you posted by commenting on this PR. In the meantime, if you have any other questions, please do not hesitate to ask.

Cheers!

timothyekl commented 8 years ago

Done in d5e4de7. Thanks for the quick reply!

radazzouz commented 8 years ago

Thanks for updating your PR so quickly, @lithium3141 👍

After much consideration and testing with my colleague @nathanvf, we concluded that this is an Awesome addition to the App Extension API. I am therefore merging this PR right now!

All the Best,