Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  Chad Pytel September 13th, 2008 @ 07:54 PM

  It appears that your patch to add :as doesn't add support for when multiple methods are being delegated to an object, such as

  
  delegate :name, :address, :to => :client
  

  Do you think we should account for this case to make this patch/feature more complete?

• 

  Pratik September 13th, 2008 @ 08:22 PM

  Don't really think this is a very useful feature. I might as well just write "def client_name;client.name;end". delegate is useful as a shortcut when you are delegating a bunch of methods to the same object.

• You flagged this item as spam.
  

  Chad Pytel September 13th, 2008 @ 08:34 PM

  I ran into this when I was exploring how best to fix Law of Demeter violations. While you can certainly do "def client_name;client.name;end", your model ends up being littered with these methods, one for each method you're delegating.

  I'd propose that in real world usage, the problem you're trying to solve with this :as option is to resolve either ambiguity in naming, or actual conflicts in naming. I think, therefore, that a better solution might be instead of :as, to add something like a :prefix option (that defaults to false for backwards compatibility) that will take either a boolean on a string/symbol, for the following functionality.

  
  class Invoice
    belongs_to :client
    delegate :name, :to => :client, :prefix => true
  end
  

  results in

  
  Invoice#client_name
  

  This would work for delegating multiple methods:

  
  class Invoice
    belongs_to :client
    delegate :name, :street, :to => :client, :prefix => true
  end
  

  results in

  
  Invoice#client_name
  Invoice#client_street
  

  and finally, I could also provide a custom prefix

  
  class Invoice
    belongs_to :client
    delegate :name, :street, :to => :client, :prefix => :customer
  end
  

  results in

  
  Invoice#customer_name
  Invoice#customer_street
  

  If it weren't for breaking code that's already using delegate, I'd say that using :prefix => true would actually be a sensible default.

• 

  Pratik September 13th, 2008 @ 08:36 PM

  :prefix does sound like a good idea.

• You flagged this item as spam.
  

  Daniel Schierbeck September 14th, 2008 @ 09:25 PM

  Chad's approach seems a lot more sensible than using the :to option. Is someone working on this? Otherwise, I could give it a stab later this week.

• You flagged this item as spam.
  

  Chad Pytel September 15th, 2008 @ 12:41 PM

  Daniel, I think you mean more sensible than the @@@:as@@@ option? If so, I probably won't have a chance to do this soon, so feel free to take a go at it.

• You flagged this item as spam.
  

  Daniel Schierbeck September 21st, 2008 @ 02:01 PM

  • Tag changed from “activesupport, patch” to “activesupport, patch”

  I've added tests and an implementation of what has been discussed here.

• You flagged this item as spam.
  

  Daniel Schierbeck September 21st, 2008 @ 02:07 PM

  Found out I could have it all in one file, sorry about the patch spam.

• You flagged this item as spam.
  

  Daniel Schierbeck September 21st, 2008 @ 02:31 PM

  Added documentation and simplified the implementation a bit.

  • delegation-prefix.diff 9.3 KB

• You flagged this item as spam.
  

  Daniel Schierbeck September 21st, 2008 @ 02:34 PM

  One question though -- what if the methods are delegated to something other than a method? A custom prefix still makes sense, but I'm not sure what the true value should do. Perhaps disallow it in such a case?

• You flagged this item as spam.
  

  Chad Pytel September 22nd, 2008 @ 02:47 PM

  I'm not sure I follow your question, or see what using the prefix of :to doesn't work in that case?

• You flagged this item as spam.
  

  Daniel Schierbeck September 22nd, 2008 @ 05:12 PM

  Chad: here's a scenario:

  class Invoice
  
    def initialize(client)
      @client = client
    end
  
    delegate :name, :address, :to => :@client, :prefix => true
  
  end
  
  

  In this case, what method names should be generated? #client_name and #client_address? What if the value of :to was a constant, e.g. SOME_CLIENT - should we lowercase the constant name?

• You flagged this item as spam.
  

  porras September 24th, 2008 @ 11:19 PM

  I created a patch which was a duplicate of this one (http://rails.lighthouseapp.com/p..., except for the :allow_nil option, which covers a regular pattern:

  
  def name
    person && person.name
  end
  

  Just in case you find it useful, I added it to this one.

  
    # If the object in which you delegate can be nil, you may want to use the
    # :allow_nil option. In that case, it returns nil instead of raising a
    # NoMethodError exception:
    #
    #  class Foo
    #    def initialize(bar = nil)
    #      @bar = bar
    #    end
    #    delegate :zoo, :to => :bar
    #  end
    #
    #  Foo.new.zoo   # raises NoMethodError exception (you called nil.zoo)
    #
    #  class Foo
    #    def initialize(bar = nil)
    #      @bar = bar
    #    end
    #    delegate :zoo, :to => :bar, :allow_nil => true
    #  end
    #
    #  Foo.new.zoo   # returns nil
  

  By the way, I'm not sure :allow_nil is the right name for this option, have you any suggestion?

• You flagged this item as spam.
  

  porras September 24th, 2008 @ 11:22 PM

  Sorry, I uploaded the wrong patch. This one is right.

• You flagged this item as spam.
  

  Daniel Schierbeck September 25th, 2008 @ 10:51 AM

  porras: I like your patch, but I think it should be separated from this one, so that comments and disagreements can be better targeted. These are effectively two different change propositions, and should be handled as such. You should add your change, with mine extracted, on top of the master branch, and submit it as a new ticket. I'd very much like to review and vote for your change. but keeping matters separate is always a good things, especially when your change doesn't depend one mine.

• You flagged this item as spam.
  

  porras September 29th, 2008 @ 12:46 PM

  daniel: you're completely right. I've already done that (http://rails.lighthouseapp.com/p..., and deleted my patch from this ticket. I'm sorry for the inconvenience =:-S

• You flagged this item as spam.
  

  Daniel Schierbeck October 11th, 2008 @ 08:02 PM

  • Title changed from “Make it possible to alias delegation methods” to “Make it possible to prefix delegation methods”
• 

  Repository October 19th, 2008 @ 12:43 PM

  • State changed from “new” to “committed”

  (from [de0ed534f6055c365d05c685582edeceef1eafa6]) Simplified the implementation of the :prefix option.

  Signed-off-by: Michael Koziarski michael@koziarski.com [#984 state:committed] http://github.com/rails/rails/co...