-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Feature - Added 204 Support to Response Serializers #889
Conversation
cc @jshier, @kcharwood, @kylef |
@@ -266,8 +348,8 @@ class ResponseSerializationTestCase: BaseTestCase { | |||
XCTAssertNotNil(result.error, "result error should not be nil") | |||
|
|||
if let error = result.error { | |||
XCTAssertEqual(error.domain, Error.Domain, "error domain should match expected value") | |||
XCTAssertEqual(error.code, Error.Code.JSONSerializationFailed.rawValue, "error code should match expected value") | |||
XCTAssertEqual(error.domain, "NSCocoaErrorDomain", "error domain should match expected value") |
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.
Can't we use the constant for NSCocoaErrorDomain
?
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.
We definitely can! Will fix.
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.
Addressed in 304af31.
XCTAssertEqual(error.domain, Error.Domain, "error domain should match expected value") | ||
XCTAssertEqual(error.code, Error.Code.JSONSerializationFailed.rawValue, "error code should match expected value") | ||
XCTAssertEqual(error.domain, NSCocoaErrorDomain, "error domain should match expected value") | ||
XCTAssertEqual(error.code, 3840, "error code should match expected value") |
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.
Isn't this NSPropertyListReadCorruptError
? Seems like a Foundation bug raising a property list error for JSON.
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.
Yeah it definitely is an NSPropertyListReadCorruptError
. Looks like they used the same errors for JSON as they did plists rather than redefining special error codes for JSON. With that said, should we keep the data.length > 0
check to instead through an actual Alamofire JSON error instead? I think it would make sense in both JSON and plist cases to keep that check. Thoughts?
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.
Been thinking about this and I think it would be much better to throw an actual Alamofire error instead of the 3840 which can be misleading. Updated in 83b32fa.
This is great 👍 |
This is great :) I just don't like the |
That's exactly right @damienrambout. These changes have a big set of tradeoffs. I'm not a huge fan of having to return the |
Feature - Added 204 Support to Response Serializers
@cnoon How I can do a request when I wait for 204 response? Can you show me a sample? |
We've been tracking this closely in our Trello project. Alamofire could greatly benefit from better 204 support. Therefore, we've been discussing the options in how to actually handle this. This PR puts forward the most sensible, non-invasive support for this behavior IMO.
The only other thing I question about this approach is whether we should require validation on the 204 as an acceptable status code before allowing this check in the response serializers. That would better help identify intent. I also hate to require that though as I feel it is a bit heavy handed.
I'm very interested to get everyone else's thoughts on this change. Anyone out there that has had to work through a 204 use case, please get your feedback in now!