Remove expand! by raising errors during expand

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

Author: mudge

README.md

@@ -44,8 +44,8 @@ uri.shortened?
 uri.expand
 #=> #<URI:HTTP http://www.altmetric.com>
 
-# Noisier expand! for explicit error handling
-Embiggen::URI('http://bit.ly/bad').expand!
+# Raises errors with bad shortened URIs
+Embiggen::URI('http://bit.ly/bad').expand
 #=> TooManyRedirects: http://bit.ly/bad redirected too many times
 # or...
 #=> BadShortenedURI: following http://bit.ly/bad did not redirect
@@ -111,9 +111,18 @@ Embiggen::URI('https://youtu.be/dQw4w9WgXcQ').expand(:redirects => 2)
 ```
 
 Expand the given URI, returning the full version as a [`URI`][URI] if it is
-shortened or the original if it cannot be expanded. Will not raise any
-exceptions thrown during expansion (e.g. timeouts, network errors, invalid
-return URIs); see `expand!` for an alternative.
+shortened or the original if it is not. Can raise the following exceptions
+during expansion:
+
+* `Embiggen::TooManyRedirects`: when a URI redirects more than the configured
+  number of times;
+* `Embiggen::BadShortenedURI`: when a URI appears to be shortened but
+  following it does not result in a redirect;
+* `Embiggen::NetworkError`: when an error occurs during expansion (e.g. a
+  network timeout, connection reset, unreachable host, etc.).
+
+All of the above inherit from `Embiggen::Error` and have a `uri` method for
+determining the problematic URI.
 
 Takes an optional hash of options for expansion:
 
@@ -124,30 +133,6 @@ Uses a whitelist of shortening domains (as configured through
 `Embiggen.configure`) to determine whether a URI is shortened or not. Be sure
 to [configure this to suit your needs](#shorteners).
 
-### `Embiggen::URI#expand!`
-
-```ruby
-Embiggen::URI('https://youtu.be/dQw4w9WgXcQ').expand!
-#=> #<URI:HTTPS https://www.youtube.com/watch?v=dQw4w9WgXcQ&feature=youtu.be>
-
-Embiggen::URI('http://bit.ly/some-bad-link').expand!
-# TooManyRedirects: http://bit.ly/some-bad-link redirected too many times
-```
-
-Expand the given URI as with `Embiggen::URI#expand` but don't suppress any
-exceptions raised during expansion (including timeouts, network errors,
-invalid return URIs, too many redirects or no redirects whatsoever).
-
-Takes the same options as `Embiggen::URI#expand`.
-
-Two Embiggen-specific errors (both inheriting from `Embiggen::Error`) can be
-raised:
-
-* `Embiggen::TooManyRedirects`: when a URI redirects more than the configured
-  number of times;
-* `Embiggen::BadShortenedURI`: when a URI appears to be shortened but
-  following it does not result in a redirect.
-
 ### `Embiggen::URI#shortened?`
 
 ```ruby

lib/embiggen/error.rb

@@ -0,0 +1,14 @@
+module Embiggen
+  class Error < ::StandardError
+    attr_reader :uri
+
+    def initialize(message, uri)
+      super(message)
+      @uri = uri
+    end
+  end
+
+  class BadShortenedURI < Error; end
+  class NetworkError < Error; end
+  class TooManyRedirects < Error; end
+end

lib/embiggen/http_client.rb

@@ -0,0 +1,33 @@
+require 'embiggen/error'
+require 'net/http'
+
+module Embiggen
+  class HttpClient
+    attr_reader :uri, :http
+
+    def initialize(uri)
+      @uri = uri
+      @http = ::Net::HTTP.new(uri.host, uri.port)
+      @http.use_ssl = true if uri.scheme == 'https'
+    end
+
+    def follow(timeout)
+      response = request(timeout)
+      return unless response.is_a?(::Net::HTTPRedirection)
+
+      response.fetch('Location')
+    rescue StandardError, ::Timeout::Error => e
+      raise NetworkError.new(
+        "could not follow #{uri}: #{e.message}", uri)
+    end
+
+    private
+
+    def request(timeout)
+      http.open_timeout = timeout
+      http.read_timeout = timeout
+
+      http.head(uri.request_uri)
+    end
+  end
+end

lib/embiggen/uri.rb

@@ -1,34 +1,26 @@
 require 'embiggen/configuration'
+require 'embiggen/error'
+require 'embiggen/http_client'
 require 'addressable/uri'
-require 'net/http'
+require 'uri'
 
 module Embiggen
   class URI
-    attr_reader :uri
+    attr_reader :uri, :http_client
 
     def initialize(uri)
       @uri = URI(::Addressable::URI.parse(uri).normalize.to_s)
+      @http_client = HttpClient.new(@uri)
     end
 
     def expand(request_options = {})
-      expand!(request_options)
-    rescue TooManyRedirects => error
-      error.uri
-    rescue StandardError, ::Timeout::Error
-      uri
-    end
-
-    def expand!(request_options = {})
       return uri unless shortened?
 
-      redirects = request_options.fetch(:redirects) { Configuration.redirects }
-      check_redirects(redirects)
+      redirects = extract_redirects(request_options)
+      location = follow(request_options)
 
-      location = head_location(request_options)
-      check_location(location)
-
-      URI.new(location).
-        expand!(request_options.merge(:redirects => redirects - 1))
+      self.class.new(location).
+        expand(request_options.merge(:redirects => redirects - 1))
     end
 
     def shortened?
@@ -37,46 +29,22 @@ def shortened?
 
     private
 
-    def check_redirects(redirects)
-      return unless redirects.zero?
-
-      fail TooManyRedirects.new("#{uri} redirected too many times", uri)
-    end
-
-    def check_location(location)
-      return if location
+    def extract_redirects(request_options = {})
+      redirects = request_options.fetch(:redirects) { Configuration.redirects }
+      fail TooManyRedirects.new(
+        "#{uri} redirected too many times", uri) if redirects.zero?
 
-      fail BadShortenedURI, "following #{uri} did not redirect"
+      redirects
     end
 
-    def head_location(request_options = {})
+    def follow(request_options = {})
       timeout = request_options.fetch(:timeout) { Configuration.timeout }
 
-      http.open_timeout = timeout
-      http.read_timeout = timeout
-
-      response = http.head(uri.request_uri)
-
-      response.fetch('Location') if response.is_a?(::Net::HTTPRedirection)
-    end
-
-    def http
-      http = ::Net::HTTP.new(uri.host, uri.port)
-      http.use_ssl = true if uri.scheme == 'https'
-
-      http
-    end
-  end
-
-  class Error < ::StandardError; end
-  class BadShortenedURI < Error; end
-
-  class TooManyRedirects < Error
-    attr_reader :uri
+      location = http_client.follow(timeout)
+      fail BadShortenedURI.new(
+        "following #{uri} did not redirect", uri) unless location
 
-    def initialize(message, uri)
-      super(message)
-      @uri = uri
+      location
     end
   end
 end