Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update configurations and unbreak compilation of a sample app. #453

Merged
merged 3 commits into from
Aug 8, 2016
Merged

Update configurations and unbreak compilation of a sample app. #453

merged 3 commits into from
Aug 8, 2016

Conversation

nlutsenko
Copy link
Contributor

@nlutsenko nlutsenko commented Jul 30, 2016 •
edited
Loading

We moved to shared configurations, but never got to updating the test chat app.

Fixes #454

richardjrossiii
richardjrossiii reviewed Aug 8, 2016
View reviewed changes
SocketRocket/Internal/Utilities/SRURLUtilities.m
@@ -55,7 +55,15 @@ extern BOOL SRURLRequiresSSL(NSURL *url)
break;
case NSURLNetworkServiceTypeVoIP: {
networkServiceType = NSStreamNetworkServiceTypeVoIP;
if (NSFoundationVersionNumber > NSFoundationVersionNumber_iOS_8_3) {

Copy link
Contributor

Choose a reason for hiding this comment

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

This completely changes the logic of the check here, near as I can tell. We probably shouldn't ever compare to NSFoundationVersionNumber10_10_Max, or DBL_MAX here. The entire check is to determine if PushKit is available, which can be done in several other ways than just a version check if we want.

The other choice being, of course, to just kill the print statement altogether, if we don't think it accomplishes anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, how is this changing the logic? I don't think I understand.

@richardjrossiii
Copy link
Contributor

Also: are the changes to SRURLUtilities relevant to this PR? I don't want to hold up unbreaking of the sample app because of some other minor changes.

@nlutsenko
Copy link
Contributor Author

Removed the print statement all together at this point.

@nlutsenko nlutsenko merged commit cccbf71 into master Aug 8, 2016
@nlutsenko nlutsenko deleted the nlutsenko.cleanup branch August 8, 2016 19:57
@nlutsenko nlutsenko added this to the 0.6.0 milestone Oct 31, 2016
madlymad pushed a commit to madlymad/SocketRocket that referenced this pull request Jul 6, 2017
@nlutsenko
Merge pull request facebookincubator#453 from facebook/nlutsenko.cleanup
16d7850 
Update configurations and unbreak compilation of a sample app.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nlutsenko nlutsenko

Labels
CLA Signed
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@nlutsenko @richardjrossiii