-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
The ASDKgram example doesn't compile. #700
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@onato Thanks for fixing this. LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this. LGTM overall, but please update indentations to match the existing convention.
@@ -428,7 +428,7 @@ | |||
GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE; | |||
GCC_WARN_UNUSED_FUNCTION = YES; | |||
GCC_WARN_UNUSED_VARIABLE = YES; | |||
IPHONEOS_DEPLOYMENT_TARGET = 8.0; | |||
IPHONEOS_DEPLOYMENT_TARGET = 9.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this file are unintentional I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentional. It doesn't compile otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
OK, I've changed the spacing to tabs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last round of questions, @onato.
@@ -428,7 +428,7 @@ | |||
GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE; | |||
GCC_WARN_UNUSED_FUNCTION = YES; | |||
GCC_WARN_UNUSED_VARIABLE = YES; | |||
IPHONEOS_DEPLOYMENT_TARGET = 8.0; | |||
IPHONEOS_DEPLOYMENT_TARGET = 9.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
completion(.failure(error)) | ||
} | ||
}.resume() | ||
DispatchQueue.main.async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation seems to be 1 tab too far?
completion(.failure(error)) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we call resume()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! Changed and retested.
…et to get the project compiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Thanks, @onato!
- Fix an insta-crash that's caused by Webservice.load method to call its completion block off the main thread. - Fix incorrect http status code check. - Bump the deployment target to get the project compiling.
It also crashes right away because it expects the Webservice.load method to return on the main thread.
I fixed these issues and an error whereby all valid responses return as failed.