Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate expand! in favour of EmbiggenedURIs #3

Closed
wants to merge 3 commits into from
Closed

Conversation

mudge
Copy link
Contributor

@mudge mudge commented Apr 19, 2015

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.

c.f. #1

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, SimpleDelegator does not 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
Copy link
Contributor Author

mudge commented Apr 19, 2015

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.

  • You should be able to create an initial instance of a URI to be expanded and tell it to expand;
  • You should then receive a consistent value object back which can be both interrogated for success and used as a URI.

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
Copy link
Contributor Author

mudge commented Apr 19, 2015

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.

@@ -18,3 +18,13 @@
mocks.verify_partial_doubles = true
end
end

RSpec::Matchers.define :embiggen do |expected|
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming feels a bit off here. expect(X).to embiggen(Y) makes it sound like Y is the thing being embiggened by X, when it's actually the result of X being embiggened.

Naming the matcher expand_to or embiggen_to would make the spec read in a more grammatically accurate way.

Rather than having Embiggen::URI delegate to the standard library URI,
restore the previous behaviour of containing a URI instead.

Now only Embiggen::EmbiggenedURI delegates to URI but does it explicitly
through Forwardable rather than SimpleDelegator: some methods are
delegated (e.g. host, port, path) but only for convenience and the true
way to get a URI instance is to use Embiggen::EmbiggenedURI#uri.

As EmbiggenedURIs no longer equate with URIs (viz.
Embiggen::EmbiggenedURI.success('http://example.com') !=
URI('http://example.com)), introduce a custom embiggen_to matcher for
the tests to abstract away the specifics of equality while still
checking the right URI is returned.
Rather than preserving only a string message describing the cause of an
expansion failure, expose the actual error object (which can then be
interrogated for its message).
@mudge
Copy link
Contributor Author

mudge commented Apr 20, 2015

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
Copy link
Contributor

👍 for removing expand! in 1.0

@mudge
Copy link
Contributor Author

mudge commented Apr 24, 2015

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

@mudge
Copy link
Contributor Author

mudge commented Apr 25, 2015

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

@mudge mudge closed this Apr 25, 2015
@mudge mudge deleted the sum-type branch April 26, 2015 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants