New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uses modern version detection API instead of floating point comparison. #13

Merged
merged 3 commits into from Apr 16, 2015

Conversation

Projects
None yet
5 participants
@steipete
Member

steipete commented Apr 14, 2015

While the current way of checking for the version is not deprecated,
converting the system version string to float isn’t a good idea either.

The framework is for iOS 8 and above, so we can use the new Foundation API.

Uses modern version detection API instead of floating point comparison.
While the current way of checking for the version is not deprecated,
converting the string to float isn’t a good idea either.

The framework is for iOS 8 and above, so we can use the new Foundation API.
@Tricertops

This comment has been minimized.

Show comment
Hide comment
@Tricertops

Tricertops Apr 14, 2015

I would make a function.

Tricertops commented Apr 14, 2015

I would make a function.

@steipete

This comment has been minimized.

Show comment
Hide comment
@steipete

steipete Apr 14, 2015

Member

I was debating extracting this into a macro or function, but wanted to keep it close to the original.

Member

steipete commented Apr 14, 2015

I was debating extracting this into a macro or function, but wanted to keep it close to the original.

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 15, 2015

Member

Yep, looks good to me, but we should also remove the #if's while we're here. The project has nullability in the headers so pre-8.3 tools cannot be used anymore.

Member

jwe-apple commented Apr 15, 2015

Yep, looks good to me, but we should also remove the #if's while we're here. The project has nullability in the headers so pre-8.3 tools cannot be used anymore.

@Tricertops

This comment has been minimized.

Show comment
Hide comment
@Tricertops

Tricertops Apr 15, 2015

What about using +instancesRespondToSelector: check? Or that UIFont method was present (and private) before iOS 8.2?

[UIFont instancesRespondToSelector:@selector(systemFontOfSize:weight:)];

Edit: Not sure what about the other cases.

Tricertops commented Apr 15, 2015

What about using +instancesRespondToSelector: check? Or that UIFont method was present (and private) before iOS 8.2?

[UIFont instancesRespondToSelector:@selector(systemFontOfSize:weight:)];

Edit: Not sure what about the other cases.

@steipete

This comment has been minimized.

Show comment
Hide comment
@steipete

steipete Apr 15, 2015

Member

@jwe-apple: re __IPHONE_8_2: Good point, overlooked that. I've updated my PR.

Member

steipete commented Apr 15, 2015

@jwe-apple: re __IPHONE_8_2: Good point, overlooked that. I've updated my PR.

@jwe-apple

This comment has been minimized.

Show comment
Hide comment
@jwe-apple

jwe-apple Apr 15, 2015

Member

@iMartinKiss Just responding to the selector is not indicative whether the API was public in this case.

Member

jwe-apple commented Apr 15, 2015

@iMartinKiss Just responding to the selector is not indicative whether the API was public in this case.

Show outdated Hide outdated ResearchKit/Common/ORKHealthAnswerFormat.m
@@ -35,9 +35,7 @@ OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
#import "ORKDefines_Private.h"
#if !defined(__IPHONE_8_2)
#define HKBiologicalSexOther 3

This comment has been minimized.

@jwe-apple

jwe-apple Apr 15, 2015

Member

Almost! Remove the HKBiologicalSexOther redefinition too, that was only for pre-8.2 tools.

@jwe-apple

jwe-apple Apr 15, 2015

Member

Almost! Remove the HKBiologicalSexOther redefinition too, that was only for pre-8.2 tools.

@steipete

This comment has been minimized.

Show comment
Hide comment
@steipete

steipete Apr 15, 2015

Member

Great. Deleting code is always fun :)

Member

steipete commented Apr 15, 2015

Great. Deleting code is always fun :)

@JGiola

This comment has been minimized.

Show comment
Hide comment
@JGiola

JGiola Apr 15, 2015

I back @iMartinKiss and even for the check if the class can register to the HKUserPreferencesDidChangeNotification notification, as for the Apple documentation https://developer.apple.com/library/ios/documentation/DeveloperTools/Conceptual/cross_development/Using/using.html is better use a control in the like of
if (&HKUserPreferencesDidChangeNotification != NULL)

JGiola commented Apr 15, 2015

I back @iMartinKiss and even for the check if the class can register to the HKUserPreferencesDidChangeNotification notification, as for the Apple documentation https://developer.apple.com/library/ios/documentation/DeveloperTools/Conceptual/cross_development/Using/using.html is better use a control in the like of
if (&HKUserPreferencesDidChangeNotification != NULL)

@YuanZhu-apple YuanZhu-apple added the ready label Apr 16, 2015

YuanZhu-apple added a commit that referenced this pull request Apr 16, 2015

Merge pull request #13 from steipete/master
Uses modern version detection API instead of floating point comparison.

@YuanZhu-apple YuanZhu-apple merged commit d56fc3e into ResearchKit:master Apr 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment