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

alias global and temporary listener methods to "subscribe" for consistancy #72

Merged
merged 1 commit into from
Aug 18, 2014

Conversation

a14m
Copy link
Contributor

@a14m a14m commented Aug 13, 2014

responding to issue #66

Please note that I'm not that experienced with the gem, so I added a this PR draft to discuss and complete the issue.
I also suspect that the testing I've done is not sufficient.
and I'm sure that the testing is not DRY... maybe moving the specs of all the 3 methods (.subscribe, .with_listeners, .add_listener) to a shared examples be better -which I plan to do before merging-

@a14m
Copy link
Contributor Author

a14m commented Aug 13, 2014

also documentation needs to be updated to indicate that

Wisper.add_listener(MyListener.new, scope: :MyPublisher)
Wisper.add_listener(MyOtherListener.new, scope: :MyPublisher)

can be used as

Wisper.subscribe(MyListener.new, MyOtherListener.new, scope: :MyPublisher)

@krisleech
Copy link
Owner

Thanks for this. I've had a quick look and my first thoughts are it looks pretty complete. I wouldn't insist on DRYing the specs, I think its probably okay as it is (open to suggestion).

I think the specs could be named better. I don't think you need to mention .with_listener and .add_listener in the descriptions as they are implementation details. Here is a suggestion:

  it '.subscribe (given block) subscribes listeners to all events for duration of the block'
  it '.subscribe subscribes listeners to all events'

For the README I'd be tempted to remove the .with_listener and .add_listener examples and just have .subscribe. In which case we could also deprecate .with_listener and .add_listener, i.e. .subscribe becomes:

def self.subscribe(*args, &block)
    if block_given?
      TemporaryListeners.with(*args, &block)
    else
      options = args.last.is_a?(Hash) ? args.pop : {}
      args.each do |listener|
        GlobalListeners.add(listener, options)
      end
    end
  end

If we did this we could also remove the specs for .{add,with}_listener making it DRY.

What do you think?

The CHANGELOG also need updating with this new feature.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 4e5625f on artmees:master into a18b215 on krisleech:master.

@a14m
Copy link
Contributor Author

a14m commented Aug 13, 2014

I've added deprecation warning for .with_listeners and .add_listener but kept their implementation as is for backwards compatibility...

I've kept the tests of both .with_listeners and .add_listener but muted the warnings in tests and added a NOTE about them being deprecated...

I've better formatted the .subscribe implementation to use the underlying classes directly (TemporaryListeners, GlobalListeners) as you mentioned.

I've also better formatted the specs for the .subscribe so that it looks similar to the following screen shot if you run specs in documentation format.

screen shot 2014-08-14 at 2 32 14 am

also If you like to remove the .with_listeners and .add_listener methods and specs please tell me and I'll update the whole PR accordingly 😄

@a14m
Copy link
Contributor Author

a14m commented Aug 14, 2014

about removing the specs for .with_listeners and .add_listener this will affect the coverage percentage... decreasing it from 100% to 83% for lib/wisper.rb. so I left them only adding Deprecation note before them...

Merging this now, closes #66

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling e14a599 on artmees:master into a18b215 on krisleech:master.

@krisleech
Copy link
Owner

Had a real quick look over this, nice work, I like how the specs read 👍

I think I'd like to {with, add}_listener method to call the new subscribe method and change their specs to just check warn is called, this might work: expect_any_instance_of(described_class).to receive(:warn).with(/deprecated/). If this proves tricky I'm happy for them to left as they are now.

Sound reasonable?

Quick tip: for commits which only change docs you can add [skip ci] to prevent CI running 😄

@a14m
Copy link
Contributor Author

a14m commented Aug 14, 2014

Done 😄, and thanks for the help...
One thing to note: the method you described to test warnings didn't work for me... (I've tried similar approaches also), but the only working solution that worked was this answer even writing a custom expectation didn't work...

I've made DEPRECATION testing in a separate commit (e294fa6) so it's easy to revert it if you don't like the change

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling e294fa6 on artmees:master into a18b215 on krisleech:master.

@krisleech
Copy link
Owner

Excellent, thanks for your contribution. I'm happy with the deprecation spec workaround. One last thing please, can you squash all the commits into one commit (and force push), then I will merge ready for the release to Rubygems next week. The commit message should be something like "add Wisper#subscribe method which replaces both .with_listeners and .add_listener [#66]". Thanks.

which replaces both .with_listeners and .add_listener
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 10bfbbf on artmees:master into a18b215 on krisleech:master.

@a14m
Copy link
Contributor Author

a14m commented Aug 17, 2014

Done 😄

krisleech added a commit that referenced this pull request Aug 18, 2014
alias global and temporary listener methods to "subscribe" for consistancy
@krisleech krisleech merged commit 5fba7cb into krisleech:master Aug 18, 2014
@krisleech
Copy link
Owner

Many thanks 😄

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

3 participants