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

Introduce Module#delegate_missing_to #23930

Merged
merged 1 commit into from May 24, 2016

Conversation

gsamokovarov
Copy link
Contributor

@gsamokovarov gsamokovarov commented Feb 27, 2016

When building decorators, a common pattern may emerge:

class Partition
  def initialize(first_event)
    @events = [ first_event ]
  end

  def people
    if @events.first.detail.people.any?
      @events.collect { |e| Array(e.detail.people) }.flatten.uniq
    else
      @events.collect(&:creator).uniq
    end
  end

  private
    def respond_to_missing?(name, include_private = false)
      @events.respond_to?(name, include_private)
    end

    def method_missing(method, *args, &block)
      @events.send(method, *args, &block)
    end
end

With Module#delegate_missing_to, the above is condensed to:

class Partition
  delegate_missing_to :@events

  def initialize(first_event)
    @events = [ first_event ]
  end

  def people
    if @events.first.detail.people.any?
      @events.collect { |e| Array(e.detail.people) }.flatten.uniq
    else
      @events.collect(&:creator).uniq
    end
  end
end

David suggested it in #23824.

When building decorators, a common pattern may emerge:

    class Partition
      def initialize(first_event)
        @events = [ first_event ]
      end

      def people
        if @events.first.detail.people.any?
          @events.collect { |e| Array(e.detail.people) }.flatten.uniq
        else
          @events.collect(&:creator).uniq
        end
      end

      private
        def respond_to_missing?(name, include_private = false)
          @events.respond_to?(name, include_private)
        end

        def method_missing(method, *args, &block)
          @events.send(method, *args, &block)
        end
    end

With `Module#delegate_missing_to`, the above is condensed to:

    class Partition
      delegate_missing_to :@events

      def initialize(first_event)
        @events = [ first_event ]
      end

      def people
        if @events.first.detail.people.any?
          @events.collect { |e| Array(e.detail.people) }.flatten.uniq
        else
          @events.collect(&:creator).uniq
        end
      end
    end

David suggested it in rails#23824.
@rails-bot
Copy link

rails-bot commented Feb 27, 2016

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

@gsamokovarov
Copy link
Contributor Author

gsamokovarov commented Feb 27, 2016

r? @dhh

@rails-bot rails-bot assigned dhh and unassigned schneems Feb 27, 2016
end

def method_missing(method, *args, &block)
#{target}.send(method, *args, &block)

Choose a reason for hiding this comment

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

Is it worth considering using public_send instead of send here?

Copy link
Member

@rafaelfranca rafaelfranca May 24, 2016

Choose a reason for hiding this comment

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

I don't think so. The delegation should be fully. Even private methods should be delegated.

Copy link
Member

@matthewd matthewd May 24, 2016

Choose a reason for hiding this comment

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

@rafaelfranca that has some pretty questionable results -- e.g., external calls to the private Kernel methods will now work on this object. I guess you want local methods to be able to call private instance methods... but IMO, it seems fair to force those to be more explicit: we're composing around the object's public API, not injecting methods inside it. Arguably, it'd be weirder if we could call private methods, yet have a separate set of ivars.

Copy link
Member

@rafaelfranca rafaelfranca May 24, 2016

Choose a reason for hiding this comment

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

oh, yeah, that is what I wanted. I agree we should use public_send here.

Copy link
Contributor

@maclover7 maclover7 May 24, 2016

Choose a reason for hiding this comment

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

Copy link
Member

@jeremy jeremy May 25, 2016

Choose a reason for hiding this comment

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

Comparing with stdlib Delegator, it sends to the target if it responds to the method (effectively a publis_send) and supers if not (so base method_missing deals with an unimplemented method rather than raising NoMethodError on our public_send).

Copy link
Member

@jeremy jeremy May 25, 2016

Choose a reason for hiding this comment

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

Seeing 3ac9956 switches to a respond_to? + public_send and supers.

@rafaelfranca rafaelfranca merged commit 335fcc2 into rails:master May 24, 2016
rafaelfranca added a commit that referenced this pull request May 24, 2016
# end
#
# The target can be anything callable withing the object. E.g. instance
# variables, methods, constants ant the likes.
Copy link
Member

@jeremy jeremy May 25, 2016

Choose a reason for hiding this comment

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

This explains what it does, but isn't enough guidance to decide whether to use delegate …, to: @events (explicitly delegate specific methods to a composed object), delegate_missing_to :@events (blanket-delegate public methods to a composed object, but don't act as a transparent proxy), or Ruby stdlib SimpleDelegator (full-class delegation to act as a transparent proxy) or Forwardable (single-method delegation like delegate …, to: …).

Copy link
Contributor Author

@gsamokovarov gsamokovarov May 25, 2016

Choose a reason for hiding this comment

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

👍 Will get on it later on tonight, unless someone beats me to it.

jeremy referenced this pull request May 25, 2016
And make sure that it doesn't even try to call the method in the target.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants