Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

switch Resource to metaprogrammed attributes to adapt to API #34

Closed
wants to merge 3 commits into from

2 participants

@bru

following the approach from the Python and PHP libraries, I switched the Ruby one to meta_program the Resource attributes.
This should make it more resilient to API changes.

poke @hmarr

@timcraft

Couple of issues with this:

  1. Plain accessors are easy to read and understand. Using method_missing and that MethodCall class to re-implement accessors makes it difficult to see what's going on.

  2. That definition of method_missing is broken because it'll fail silently when calling it with something that isn't an assignment. For example, calls like these should raise NoMethodError:

    some_resource.something!
    some_resource.something?
    some_resource.some_method_that_is_not_an_attribute

I can understand the motivation for wanting the resources to automatically detect new attributes. Alternative suggestion: set instance variables in the constructor (instead of using send), keep the explicit accessors, and use a simplified method_missing to provide read only access to attributes in the data that don't yet have accessors.

Cheers,
Tim

@bru
bru commented

Yep, thanks for pointing out the method_missing shortcomings. I'll amend those.
I'm not a fan of metaprogramming either, but I'm also concerned about inconsistency in the approach amongst the different libraries. Tough choice.

Cheers

@bru bru Fix method_missing to follow conventions
* trying to access a non-existing attribute will raise NoMethodError
* #valid_attribute? will return true if the attribute isn't nil?
* #valid_attribute! behaves as #attribute
852f16a
@bru
bru commented

The last commit should fix the method_missing issues.
Still considering the attr_accessor thing.

@bru
bru commented

dammit. issues with ruby 1.8 :)

@bru
bru commented

ok, this looks a bit too dangerous as it is now:
raising the NoMethodError on missing attributes may break existing applications (e.g. if the API responses don't return nil attributes).
I'm closing this pull request and going to double check the attr accessors instead.

@bru bru closed this
@timcraft

If by consistency you mean "feature parity" (i.e. that the different libraries should have the same features), then that makes sense, but those features don't need to be coded in the same style. Better to use the idioms that the language gives you. When in Ruby speak Ruby, when in Python speak Python etc.

The underlying issue is a potential mismatch between attributes defined in the library and attributes in use by the HTTP API. The API should be backwards compatible, so it's only new attributes that need to be added. A simple solution to this is just to regularly update the library and release a new version with the new attributes. Anyone who cannot wait a few weeks to update to the new version can monkey-patch them in temporarily like this:

class GoCardless::Bill
  attr_accessor :gocardless_fees
  attr_accessor :partner_fees
end

Not sure how your responsibilities are spread out internally, but you could potentially make the developer who is responsible for adding/deploying the attribute change to the HTTP API also responsible for updating the libraries. Or have an official maintainer for each library and their responsibility is to keep in sync with the HTTP API changes.

Cheers,
Tim

@bru bru deleted the branch
@bru
bru commented
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 29, 2013
  1. @bru
Commits on Feb 5, 2013
  1. @bru

    Fix method_missing to follow conventions

    bru authored
    * trying to access a non-existing attribute will raise NoMethodError
    * #valid_attribute? will return true if the attribute isn't nil?
    * #valid_attribute! behaves as #attribute
  2. @bru
This page is out of date. Refresh to see the latest.
View
11 lib/gocardless/bill.rb
@@ -4,17 +4,6 @@ class Bill < Resource
creatable
- attr_accessor :amount,
- :source_type,
- :description,
- :name,
- :plan_id,
- :status
-
- # @attribute source_id
- # @return [String] the ID of the bill's source (eg subscription, pre_authorization)
- attr_accessor :source_id
-
reference_accessor :merchant_id, :user_id, :payment_id
date_accessor :created_at, :paid_at
View
9 lib/gocardless/merchant.rb
@@ -2,15 +2,6 @@ module GoCardless
class Merchant < Resource
self.endpoint = '/merchants/:id'
- attr_accessor :name,
- :description,
- :email,
- :first_name,
- :last_name,
- :balance,
- :pending_balance,
- :next_payout_amount,
- :hide_variable_amount
date_accessor :created_at, :next_payout_date
end
end
View
1  lib/gocardless/payment.rb
@@ -2,7 +2,6 @@ module GoCardless
class Payment < Resource
self.endpoint = '/payments/:id'
- attr_accessor :amount, :currency, :status
reference_accessor :merchant_id, :user_id
date_accessor :created_at
end
View
11 lib/gocardless/pre_authorization.rb
@@ -2,17 +2,6 @@ module GoCardless
class PreAuthorization < Resource
self.endpoint = '/pre_authorizations/:id'
- attr_accessor :max_amount,
- :currency,
- :amount,
- :interval_length,
- :interval_unit,
- :name,
- :description,
- :plan_id,
- :status,
- :remaining_amount
-
reference_accessor :merchant_id, :user_id
date_accessor :expires_at, :created_at, :next_interval_start
View
44 lib/gocardless/resource.rb
@@ -33,7 +33,49 @@ def initialize(hash = {})
end
# Set resource attribute values
- hash.each { |key,val| send("#{key}=", val) if respond_to?("#{key}=") }
+ hash.each { |key,val| send("#{key}=", val) }
+ end
+
+ ##
+ # parses a method call passed to method_missing
+ class MethodCall
+ attr_accessor :attribute
+ def initialize(string)
+ all, @attribute, @operation = *(/([^=!\?]+)([=!\?]?)/.match string.to_s)
+ end
+
+ def assignment?
+ @operation == "="
+ end
+
+ def check?
+ @operation == "?"
+ end
+ end
+
+ # sets or gets an instance variable for this resource
+ def method_missing(method, *args)
+ op = MethodCall.new(method)
+ if op.assignment?
+ instance_variable_set("@#{op.attribute}", args.first)
+ elsif op.check?
+ check_attribute(op.attribute)
+ else
+ get_attribute(op.attribute)
+ end
+ end
+
+ def valid_attribute?(attr)
+ instance_variables.map(&:to_s).include? "@#{attr}"
+ end
+
+ def get_attribute(attr)
+ raise NoMethodError unless valid_attribute?(attr)
+ instance_variable_get("@#{attr}")
+ end
+
+ def check_attribute(attr)
+ !get_attribute(attr).nil?
end
attr_writer :client
View
13 lib/gocardless/subscription.rb
@@ -3,23 +3,10 @@ class Subscription < Resource
self.endpoint = '/subscriptions/:id'
- attr_accessor :amount,
- :currency,
- :interval_length,
- :interval_unit,
- :name,
- :description,
- :plan_id,
- :status,
- :setup_fee,
- :trial_length,
- :trial_unit
-
reference_accessor :merchant_id, :user_id
date_accessor :start_at, :expires_at, :created_at, :next_interval_start
-
def cancel!
path = self.class.endpoint.gsub(':id', id.to_s) + '/cancel'
client.api_put(path)
View
1  lib/gocardless/user.rb
@@ -2,7 +2,6 @@ module GoCardless
class User < Resource
self.endpoint = '/users/:id'
- attr_accessor :name, :first_name, :last_name, :email
date_accessor :created_at
end
end
View
23 spec/resource_spec.rb
@@ -2,14 +2,31 @@
describe GoCardless::Resource do
it "initializes from hash" do
- test_resource = Class.new(GoCardless::Resource) do
- attr_accessor :id, :name, :uri
- end
+ test_resource = Class.new(GoCardless::Resource)
+
props = {:id => 1, :name => 'test', :uri => 'http://test'}
resource = test_resource.new(props)
props.each { |k,v| resource.send(k).should == v }
end
+ describe "dynamic attribute accessors" do
+ it "raises NoMethodError on non-existing attributes" do
+ test_resource = Class.new(GoCardless::Resource)
+
+ props = { :id => 1, :name => 'test', :uri => 'http://test' }
+ resource = test_resource.new(props)
+ expect { resource.not_existing }.to raise_error(NoMethodError)
+ end
+
+ it "attribute! behaves like attribute" do
+ test_resource = Class.new(GoCardless::Resource)
+
+ props = { :id => 1, :name => 'test', :uri => 'http://test' }
+ resource = test_resource.new(props)
+ props.each { |k,v| resource.send("#{k}!").should == v }
+ end
+ end
+
describe "#date_writer" do
it "creates date writers properly" do
test_resource = Class.new(GoCardless::Resource) do
Something went wrong with that request. Please try again.