Skip to content

[Swift] Include HTTP status code and body data in response errors#3624

Merged
wing328 merged 3 commits intoswagger-api:masterfrom
ButterflyNetwork:swift-better-errors
Aug 24, 2016
Merged

[Swift] Include HTTP status code and body data in response errors#3624
wing328 merged 3 commits intoswagger-api:masterfrom
ButterflyNetwork:swift-better-errors

Conversation

@jgavris
Copy link
Copy Markdown
Contributor

@jgavris jgavris commented Aug 22, 2016

This is useful when differentiating different error responses. Example:

if let e = error {
  switch e {
    case ErrorResponse.Error(400, _, _): responseLabel.text = "Unknown error"
    case ErrorResponse.Error(403, _, _): responseLabel.text = "Not logged in"
    default: responseLabel.text = "(e)"
  }
}

The existing error is straight from Alamofire and only includes the HTTP
status code in the body of the message (not broken out as a separate value).
It also does not include the response body, which may be useful.

This is useful when differentiating different error responses. Example:

if let e = error {
  switch e {
    case ErrorResponse.Error(400, _, _): responseLabel.text = "Unknown error"
    case ErrorResponse.Error(403, _, _): responseLabel.text = "Not logged in"
    default: responseLabel.text = "\(e)"
  }
}

The existing error is straight from Alamofire and only includes the HTTP
status code in the body of the message (not broken out as a separate value).
It also does not include the response body, which may be useful.
expectation.fulfill()

switch error {
case ErrorResponse.Error(200, _, _):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will there ever be an error w/ a 200 status? This reads a little odd here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree, this is awkard. It's result of the current swift implementation, which doesn't handle Void responses currently. Let me see if I can make that go away.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jaz-ah @wing328 Updated. This is now a breaking change with Respose.body being optional now to handle Void responses. Covers all the petstore examples without ugly hacks now :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

much better thx!

@jgavris jgavris force-pushed the swift-better-errors branch from 8428075 to 1eb7629 Compare August 23, 2016 14:16
@jgavris jgavris force-pushed the swift-better-errors branch from 1eb7629 to e89f914 Compare August 23, 2016 14:18
XCTFail("error logging out")
}
PetAPI.deletePet(petId: 1000).then {
expectation.fulfill()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we still have to do some type of error checking here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's no more error, and we're using the then block.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah ok cool thx

@jaz-ah
Copy link
Copy Markdown
Contributor

jaz-ah commented Aug 23, 2016

+1 good to go @wing328 - thx for the contribution @jgavris !

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Aug 24, 2016

Thanks all for the work. All the Petstore tests passed locally.

@wing328 wing328 merged commit 8c2267c into swagger-api:master Aug 24, 2016
@jgavris jgavris deleted the swift-better-errors branch August 24, 2016 15:40
@jgavris
Copy link
Copy Markdown
Contributor Author

jgavris commented Aug 24, 2016

🎈

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants