altmetric / embiggen

A Ruby library to expand shortened URLs
https://rubygems.org/gems/embiggen
MIT License
124 stars 6 forks source link

Deprecate expand! in favour of EmbiggenedURIs #3

Closed mudge closed 9 years ago

mudge commented 9 years ago

As discussed in #1, rather than having two expansion methods (one gracefully handling errors but not communicating any information about them and one eagerly raising exceptions), consistently return a sum type from expand that both encapsulates exceptions and communicates success:

uri = Embiggen::URI('http://examp.le/badlink').expand
#=> #<Embiggen::EmbiggenedURI http://examp.le/badlink>
uri.success?
#=> true or false
uri.reason
#=> 'following http://examp.le did not redirect'
uri.expand

This allows us to deprecate expand! but increases the amount of code for now (to accommodate the exceptions it raises).

An important change is that Embiggen::URI now delegates to its underlying URI object and Embiggen::EmbiggenedURI in turn delegates to an instance of Embiggen::URI. This creates the following delegation hierarchy:

     URI::Generic
         |
   Embiggen::URI
         |
Embiggen::EmbiggenedURI

However, neither SimpleDelegator nor DelegateClass forward is_a? so the return types do not return true for uri.is_a?(URI::HTTP), for example.

It may be that relying on delegation is a bad idea or, to take the other extreme, perhaps EmbiggenedURI should truly inherit from URI::HTTP.

mudge commented 9 years ago

I should note that I'm opening this more for discussion than anything else: the tension between composition and inheritance (and using delegation as a way of cheating) doesn't seem right to me here.

Perhaps it is too much responsibility for the return value of expand to be a URI and a response object and instead the API should be like the following?

uri = Embiggen::URI('http://foo.bar').expand
uri.success?
#=> true
uri.uri
#=> #<URI::HTTPS https://fooish.barish>

uri = Embiggen::URI('http://foo.bar').expand
uri.success?
#=> false
uri.uri
#=> #<URI::HTTP http://foo.bar>
uri.reason
#=> 'execution expired'
uri.error
#=> #<Timeout::Error ...>

Then if you want to continue expanding, you could pass it to Embiggen::URI again:

uri = Embiggen::URI('http://foo.bar').expand
uri.success?
#=> false
uri_again = Embiggen::URI(uri.uri).expand
mudge commented 9 years ago

Now updated to no longer delegate to URI so aggressively but instead encapsulate URI instances:

uri = Embiggen::URI('http://examp.le/123').expand
#=> #<Embiggen::EmbiggenedURI http://www.example.com/some-long-url?a=1#b>

EmbiggenedURI now contains a real URI instance alongside its success and error (in case of failure) but delegates specific methods to its URI for convenience:

uri.to_s
#=> 'http://www.example.com/some-long-uri?a=1#b'
uri.scheme
#=> 'http'
uri.host
#=> 'www.example.com'
uri.path
#=> '/some-long-uri'
uri.request_uri
#=> '/some-long-uri?a=1'
uri.query
#=> 'a=1'
uri.fragment
#=> 'b'

Of course, you can get the actual instance out for anything else:

uri.uri
#=> #<URI::HTTP http://www.example.com/some-long-uri>

EmbiggenedURIs are not Embiggen::URIs and can't be asked if they are shortened? or told to expand without passing back to Embiggen.URI.

mudge commented 9 years ago

It's worth noting that we could completely remove expand! (and skip the deprecation warning) if we bump the major version of the gem (we're currently on 0.1 so we'd have to go to at least 1.0). This would probably clean up a lot of embiggen/uri.rb at the cost of backward compatibility.

scottmatthewman commented 9 years ago

:+1: for removing expand! in 1.0

mudge commented 9 years ago

See https://github.com/altmetric/embiggen/compare/sum-type-with-addressable for a version of this with #6 rebased into it.

mudge commented 9 years ago

Closing this as I'll re-open against the 1.x branch with the changes from #7.