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

Simplify interface to Configuration.shorteners #2

Closed
mudge opened this issue Apr 18, 2015 · 1 comment
Closed

Simplify interface to Configuration.shorteners #2

mudge opened this issue Apr 18, 2015 · 1 comment

Comments

@mudge
Copy link
Contributor

mudge commented Apr 18, 2015

As raised by @sferik on Twitter:

I’d encourage you to reconsider the necessity of the existence of the shorteners class method. It will be a maintenance nightmare.

It still seems valuable to provide a default list of shorteners but to encourage users to supply their own.

The current implementation interrogates Configuration.shorteners with any? and expects to be yielded a series of domains that respond to to_s.

If the interface between Embiggen::URI and Configuration.shorteners was simplified to include? (or something similar) then it would be possible to replace a static list of shorteners with much more powerful options, e.g.

require 'bitly'

class BitlyDomains
  extend Forwardable
  attr_reader :client
  def_delegator :client, :pro?, :include?

  def initialize(client)
    @client = client
  end
end

Bitly.use_api_version_3
Bitly.configure do |config|
  config.api_version = 3
  config.access_token = ENV.fetch('BITLY_ACCESS_TOKEN')
end

Embiggen.configure do |config|
  config.shorteners = BitlyDomains.new(Bitly.client)
end
mudge added a commit that referenced this issue Nov 21, 2015
GitHub: #2

Allow users to supply their own object for
Embiggen::Configuration.shorteners that only responds to include? to
determine whether a URI is shortened or not.

This replaces the current Set implementation with a ShortenerList class
that has the same API (to keep testing simple) but exposes a custom
include? method.
@mudge
Copy link
Contributor Author

mudge commented Nov 21, 2015

Fixed in 1.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant