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

Safely prepend Publisher initializers #3

Merged
merged 2 commits into from Mar 30, 2014
Merged

Safely prepend Publisher initializers #3

merged 2 commits into from Mar 30, 2014

Conversation

ntl
Copy link
Contributor

@ntl ntl commented Mar 29, 2014

Lazy load ivars previously created in initializers

This prevents the initializers in the modules from colliding with initializers in the included class, but without using Module#prepend, which is not available in ruby 1.9 or current jruby.

EDIT: the initial approach used Module#prepend which was abandoned in favor of removing the initializers in the modules altogether.

@karmajunkie
Copy link
Owner

Its a good PR, but I'm a little wary of the Ruby 2 dependency. Seen any numbers on R2.0 penetration in the market? One of my goals for this is to be able to use this pattern on legacy applications.

I'm also going to experiment with an alternate implementation that may obviate this—basically, apply would create an anonymous class which is used to store the state of the domain object, and each apply block would be a method implemented on that object. Therefore, no initializer override or prepend needed, and it would work on all versions of ruby.

@ntl
Copy link
Contributor Author

ntl commented Mar 30, 2014

I don't know the penetration, but ruby 1.9 has less than a year before it's end of lifed. People should be migrating off today, if they haven't already. Also, the upgrade path from 1.9 -> 2.0 isn't nearly as painful as 1.8 -> 1.9.

As a short term solution to avoid #prepend, we could stick @subscription_manager behind a memoized instance method:

module Publisher
  def subscription_manager
    @subscription_manager ||= Replay::SubscriptionManager.new(Replay.logger)
  end
  private :subscription_manager
end

This may fix the problem that's blocking me without hosing ruby 1.9. I generally dislike this kind of lazy loading, but it may be the best choice.

@karmajunkie
Copy link
Owner

Well, 1.9 isn't the only non-implementer of #prepend—JRuby doesn't implement it right now either. Not sure about Rubinius.

@ntl
Copy link
Contributor Author

ntl commented Mar 30, 2014

Rubinius seems to be compatible with mri 2.1. Would you be okay with the lazy loading approach I mentioned? If so, I can resubmit the PR. The bug caused by the existing implementation is a show stopper for me.

@karmajunkie
Copy link
Owner

yeah if you resubmit it with the lazy loading approach i'll accept it

ntl added 2 commits March 30, 2014 16:54
This change alters `Publisher` and `EventExaminer` to use
`Module#prepend` to inject their initializers into the call chain. The
intention is to allow classes that include `Publisher` to include their
own `#initialize` method that still preserves the behavior of
`Publisher#initialize`.

Consider:

    module Foo
      def initialize(*args)
        puts "Foo: #{args.inspect}"
        super
      end
    end

    class A
      include Foo
      def initialize(arg)
        puts "A: #{arg}"
      end
    end

    class B
      prepend Foo
      def initilaize(arg)
        puts "B: #{arg}"
      end
    end

In the case of `A`, since `Foo` is attached via `#include`, the
initializer defined in `Foo` is essentially ignored:

    irb(main):046:0> A.new :foo
    A: :foo
    => #<A:0x007fae191b4a08>

However, in `B`, since `Foo` is attacted via `#prepend`, the initializer
actually ends up behaving as if it were a superclass method of
`B#initialize`:

    irb(main):045:0> B.new :foo
    Foo: [:foo]
    B: :foo
    => #<B:0x007fae191bca28>

This `#prepend` trick works well for your initializers that actually
want to inject behavior during initialization, but ultimately stay out
of the way of any `#initialize` defined by the consuming class.

I ran into this when I tried including `Publisher` in a class that had
an initializer with arguments; I was seeing a "1 for 0" `ArgumentError`.
When I dug into the proofs, I noticed that `ReplayTest` does not
actually have an `#initialize` method of its' own.

My initial effort at producing a failing test was to subclass
`ReplayTest` within the specific proof for testing custom initializers;
however, subclassing overrode the faulty behavior and thus did not
produce a failing test. Instead, I added a constructor argument to
override the `pkey` attribute. It seemed like the least janky way to
introduce a failing test. If I made the proofs worse, I can try and
think of a better way to expose the problem I was trying to solve.

It's worth noting that this change would break ruby 1.9, which does not
have `Module#prepend`.
This prevents the initializers in the modules from colliding with
initializers in the included class, but *without* using Module#prepend,
which is not available in ruby 1.9 or current jruby.
@ntl
Copy link
Contributor Author

ntl commented Mar 30, 2014

Cool, thanks!

karmajunkie added a commit that referenced this pull request Mar 30, 2014
Safely prepend Publisher initializers
@karmajunkie karmajunkie merged commit 2a1a4c2 into karmajunkie:master Mar 30, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants