Skip to content
This repository has been archived by the owner on Nov 21, 2018. It is now read-only.

Add StubAdapter #26

Merged
merged 13 commits into from
Oct 29, 2015
Merged

Add StubAdapter #26

merged 13 commits into from
Oct 29, 2015

Conversation

kayvonghaffari
Copy link
Contributor

[#21]

@roberthoner
Copy link
Contributor

As a part of this change, can we standardize on how the version is specified. Currently, with HttpAdapter, the version is assumed to already be in the url. In the LocalAdapter, the version must be passed as param. In this current SubAdapter implementation, versions aren't handled.

Let's standardize on how LocalAdapter does this:

Rester.connect(SERVICE, version: NUM)

If SERVICE is a Rester::Service then it will use the LocalAdapter. If it is a URL, it will use the HttpAdapter. If it is a file path, it'll use the StubAdapter.

In every case, the client should now start reading in the version param and ensuring that when the adapter is called, the version is prepended.

I'm thinking something like:

# client.rb
module Rester
  class Client
    def request(verb, path, params={}, &block)
      path = _path_with_version(path) # New helper method.

      # This line is the same as what's currently in this method.
      _process_response(path, *adapter.request(verb, path, params, &block))
    end
  end
end

unless (file_ext = Pathname(args.first).extname) == '.yml'
raise Errors::InvalidStubFileError, "Expected .yml, got #{file_ext}"
end
Client.new(Client::Adapters::StubAdapter.new(*args))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this method do be more explicit instead of loose wrapper around Client.

For example:

def connect(service, params={})
  if service.is_a?(Class) && service < Service
    adapter = Client::Adapters::LocalAdapter.new(service)
  elsif service.is_a?(String) && Pathname(service).file?
    adapter = Client::Adapters::StubAdapter.new(service)
  elsif service.is_a?(String) && ['http', 'https'].include?(URI(service).scheme)
    adapter = Client::Adapters::HttpAdapter.new(service)
  else
    fail "unable to connect to: #{service.inspect}"
  end

  Client.new(adapter, params)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

much better

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better?

def connect(service, params={})
  adapter_klass = Client::Adapters.list.find { |a| a.can_connect_to?(service) }
  fail "unable to connect to: #{service.inspect}" unless adapter_klass
  adapter = adapter_klass.new(service)
  Client.new(adapter, params)
end

Then in each adapter class, define a class method ::can_connect_to? that will do whatever checks are necessary.

Also, define an Client::Adapters.list method that provides a list of all the adapters (this could be dynamic based on what's in the adapters/ directory.

@kayvonghaffari
Copy link
Contributor Author

Finished the necessary changes @roberthoner

@roberthoner
Copy link
Contributor

I'm good to merge this @kayvonghaffari

@kayvonghaffari
Copy link
Contributor Author

merging per @roberthoner 's approval

kayvonghaffari added a commit that referenced this pull request Oct 29, 2015
@kayvonghaffari kayvonghaffari merged commit 2e6ae90 into master Oct 29, 2015
@roberthoner roberthoner deleted the issue/21 branch December 10, 2015 20:00
@roberthoner roberthoner modified the milestone: 0.4.3 Dec 10, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants