This repository has been archived by the owner on Apr 29, 2019. It is now read-only.
Improve Error Messages and Recovery Suggestions #44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolves #35 `Improve Error Explanation for Rate Limit`. - Adds checks for rate limiting and uses new, specific error - Refactors response validation to static methods in an extension of `ResponseHandler`, thus providing a default implementation to all response handlers - Refactors the deserialisation of JSON in `JSONResponseHandler` into a separate method to improve readability - Updates `GitHubScannerProtocolError` to inherit from `LocalizedError` and provides a default implementation for `CustomStringConvertible` - Adds `errorDescription` and `recoverySuggestion` to all error types - Moves `NetworkError` and `ScanOptionsError` into own files - Expands test suite for error handling - Refactors accessing the `HTTPURLResponse` next link header into an extension property to improve readability of `JSONResponseHandler` and to help maintain single responsibility principle
ghost
requested review from
arnau
approved these changes
Feb 20, 2017
| case .unauthorized: | ||
| return "Not authorized" | ||
| case let .unknown(error): | ||
| if let localError = error as? LocalizedError, |
| } | ||
| } | ||
|
|
||
| public var recoverySuggestion: String? { |
| /// - Parameter response: The `HTTPURLResponse` to validate. | ||
| /// - Returns: Whether or not the response is valid. | ||
| public static func isValidResponseStatus(_ response: HTTPURLResponse) -> Bool { | ||
| return 200..<300 ~= response.statusCode |
There was a problem hiding this comment.
The 3xx range is valid as well. Perhaps in a future iteration you should handle at least a few of them (i.e. 301, 302 and 307) https://developer.github.com/v3/#http-redirects
There was a problem hiding this comment.
Added Issue #45 Handle HTTP Redirects.
Per code review by @arnau
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #35
Improve Error Explanation for Rate Limit.ResponseHandler, thus providing a default implementation to all response handlersJSONResponseHandlerinto a separate method to improve readabilityGitHubScannerProtocolErrorto inherit fromLocalizedErrorand provides a default implementation forCustomStringConvertible. This allows displaying of the error message and recovery suggestion to the user ofgithub-scannererrorDescriptionandrecoverySuggestionto all error typesNetworkErrorandScanOptionsErrorinto own filesHTTPURLResponsenext link header into an extension property to improve readability ofJSONResponseHandlerand to help maintain single responsibility principle