Skip to content

NotificationCenter should accept any Callable #216

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

Merged
merged 7 commits into from
Dec 3, 2019

Conversation

jasonkarns
Copy link
Contributor

Summary

The NotificationCenter learns to accept any callable object as the
notification callback.

It is uncommon in Ruby to require Method instances when any Callable
will suffice. Indeed, the idiomatic object for callbacks is a block.

With this change, the notification center can accept any callable, not
only Method instances; that is, any object that responds to call
(Method, lambdas, procs, or any object with a .call method)

If the callback parameter is omitted, a block may be provided instead.

Test plan

Issues

address item 3 of #213

@jasonkarns
Copy link
Contributor Author

Not sure what's going on with Travis. The failure is occurring before the test step even begins. https://travis-ci.org/optimizely/ruby-sdk/jobs/602833534

@brianlucas
Copy link

@jasonkarns I apologize for the delay in getting back to you on this.
This also looks good to me, and thanks for adding the test cases to accommodate the proposed implementation. One quick question - do you think it would be beneficial to provide a Proc.new {} test case as well (which also responds to :call)?

Happy to approve once rebase/remerged.

cc @mjc1283

@jasonkarns
Copy link
Contributor Author

do you think it would be beneficial to provide a Proc.new {} test case as well (which also responds to :call)?

I can, but I don't think that additional test is valuable. We're already asserting that Proc instances can be provided (with the lambda test). It doesn't confirm anything that isn't already manifested in the tests and the lambda generation is more strict anyway (since the generated Proc checks arity).

Put another way, it's generally preferable to have only a single test fail for any given bug. Adding another test that fails for the same reason as an existing test doesn't provide value commensurate to its maintenance/carrying cost.

Copy link

@brianlucas brianlucas left a comment

Choose a reason for hiding this comment

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

lgtm

@msohailhussain
Copy link
Contributor

@jasonkarns can you please review this PR and merge it in this branch. It has lint fixes that should be resolved first.
jasonkarns#1

@jasonkarns
Copy link
Contributor Author

Merged! (feel free to make any future changes like that directly on the branch. I enabled "accept edits by maintainers" to facilitate this)

image

@msohailhussain
Copy link
Contributor

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

5 participants