Remove expand! by raising errors during expand #10
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
mudge
changed the title
expand! by raising errors during expand
|
looks good to me 👍 |
| def extract_redirects(request_options = {}) | ||
| redirects = request_options.fetch(:redirects) { Configuration.redirects } | ||
| fail TooManyRedirects.new( | ||
| "#{uri} redirected too many times", uri) if redirects.zero? |
There was a problem hiding this comment.
Correct me if I'm wrong, but at this point uri is not the original one anymore, so the message is not entirely correct.
There was a problem hiding this comment.
That's true: it's the last URI followed.
Perhaps it's just the message that needs correcting here as the initial URI is in the original Embiggen::URI instance but it seems useful to show how much progress was made.
There was a problem hiding this comment.
👍 sth like "Following #{uri} reached the redirects limit"
|
Otherwise, looks good to me! |
GitHub: #1 Rather than having separate expand and expand! methods (one swallowing all exceptions, one not), simplify the API of Embiggen::URI to a single expand method. This no longer aggressively swallows exceptions outside of its control but may raise subclasses of Embiggen::Error, namely: * Embiggen::BadShortenedURI; * Embiggen::TooManyRedirects; * Embiggen::NetworkError. The last of these wraps any network-related exceptions including timeouts, connection resets, unreachable hosts, etc. The reason for this change is that these truly are exceptional circumstances outside of the library's control (unlike, say, an unshortened URI being passed to Embiggen::URI which can be handled gracefully). The previous API took too much responsibility away from the client by silently discarding exceptions by default and therefore obscuring failures. Originally, the idea of using a [sum type][0] was appealing (e.g. returning some sort of result object which could be interrogated for its success) but this later seemed overengineered and unidiomatic. Instead, we prefer to return standard Ruby types where appropriate (e.g. standard library URIs) and raise exceptions in case of failure. This simplification lead to a refactoring that extracted an HttpClient class which is currently used privately within the library. This potentially opens the way for pluggable HTTP clients (rather than being forced to use Net::HTTP) in future (c.f. #9). [0]: http://en.wikipedia.org/wiki/Tagged_union
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
GitHub: #1
Rather than having separate
expandandexpand!methods (one swallowing all exceptions, one not), simplify the API ofEmbiggen::URIto a singleexpandmethod. This no longer aggressively swallows exceptions outside of its control but may raise subclasses ofEmbiggen::Error, namely:Embiggen::BadShortenedURI;Embiggen::TooManyRedirects;Embiggen::NetworkError.The last of these wraps any network-related exceptions including timeouts, connection resets, unreachable hosts, etc.
The reason for this change is that these truly are exceptional circumstances outside of the library's control (unlike, say, an unshortened URI being passed to
Embiggen::URIwhich can be handled gracefully). The previous API took too much responsibility away from the client by silently discarding exceptions by default and therefore obscuring failures.Originally, the idea of using a sum type was appealing (e.g. returning some sort of result object which could be interrogated for its success) but this later seemed overengineered and unidiomatic. Instead, we prefer to return standard Ruby types where appropriate (e.g. standard library URIs) and raise exceptions in case of failure.
This simplification lead to a refactoring that extracted an
HttpClientclass which is currently used privately within the library. This potentially opens the way for pluggable HTTP clients (rather than being forced to useNet::HTTP) in future (c.f. #9).