This project is archived and is in readonly mode.

#984 ✓committed
Daniel Schierbeck

Make it possible to prefix delegation methods

Reported by Daniel Schierbeck | September 6th, 2008 @ 05:24 PM | in 2.x

This patch allows one to prefix delegation methods, either with a custom string or with the name of the delegation target.

class Invoice

  attr_accessor :client

  # Creates #client_name and #client_address
  delegate :name, :address :to => :client, :prefix => true

  # Creates #customer_name and #customer_address
  delegate :name, :address :to => :client, :prefix => :customer

end

Comments and changes to this ticket

  • Chad Pytel

    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

    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.

  • Chad Pytel

    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

    Pratik September 13th, 2008 @ 08:36 PM

    :prefix does sound like a good idea.

  • Daniel Schierbeck

    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.

  • Chad Pytel

    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.

  • Daniel Schierbeck

    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.

  • Daniel Schierbeck

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

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

  • Daniel Schierbeck

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

    Added documentation and simplified the implementation a bit.

  • Daniel Schierbeck

    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?

  • Chad Pytel

    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?

  • Daniel Schierbeck

    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?

  • porras

    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?

  • porras

    porras September 24th, 2008 @ 11:22 PM

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

  • Daniel Schierbeck

    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.

  • porras

    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

  • Daniel Schierbeck

    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

    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...

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Referenced by

Pages