Skip to content

Implements callback closures #20

Open
rashidsp wants to merge 5 commits intomasterfrom
notificaton-listener/closures
Open

Implements callback closures #20
rashidsp wants to merge 5 commits intomasterfrom
notificaton-listener/closures

Conversation

@rashidsp
Copy link
Copy Markdown
Collaborator

@rashidsp rashidsp commented Dec 12, 2017

No description provided.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 12, 2017

Coverage Status

Coverage decreased (-0.03%) to 99.563% when pulling 622237c on notificaton-listener/closures into 3c93241 on master.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.2%) to 99.781% when pulling fb3cb04 on notificaton-listener/closures into 3c93241 on master.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 14, 2017

Coverage Status

Coverage increased (+0.2%) to 99.743% when pulling 83a6e23 on notificaton-listener/closures into 96bf517 on master.

oakbani
oakbani requested changes Dec 14, 2017
View reviewed changes
Copy link
Copy Markdown
Collaborator

@oakbani oakbani left a comment

Choose a reason for hiding this comment

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

Please see if we can accomodate block of code as param to add_notification such as
http://www.mattsears.com/articles/2011/11/27/ruby-blocks-as-dynamic-callbacks/

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 14, 2017

Coverage Status

Coverage increased (+0.1%) to 99.672% when pulling c12d6b9 on notificaton-listener/closures into 96bf517 on master.

@rashidsp
Copy link
Copy Markdown
Collaborator Author

rashidsp commented Dec 14, 2017

@oakbani Implemented block and related unit tests. Please review.

@rashidsp
Copy link
Copy Markdown
Collaborator Author

rashidsp commented Dec 14, 2017

@msohailhussain It's ready. Should we create PR to Optimizely?

msohailhussain
msohailhussain reviewed Dec 14, 2017
View reviewed changes
lib/optimizely/notification_center.rb
end

def add_notification_listener(notification_type, notification_callback)
def add_notification_listener(notification_type, callback = nil, &block)
Copy link
Copy Markdown
Owner

@msohailhussain msohailhussain Dec 14, 2017

Choose a reason for hiding this comment

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

this is wrong approach.
either you should add two listeners method, not default argument.

Copy link
Copy Markdown
Collaborator Author

@rashidsp rashidsp Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ruby does not support method overloading.
We could only send in default arguments and the other way is like 'method_name(*args)'.
https://www.ruby-forum.com/topic/890619
Proc and lambda are used as closures to send a block in arguments, which is already being used.

msohailhussain
msohailhussain reviewed Dec 14, 2017
View reviewed changes
lib/optimizely/notification_center.rb

return nil unless notification_type_valid?(notification_type)

notification_callback = callback.present? ? callback : block
Copy link
Copy Markdown
Owner

@msohailhussain msohailhussain Dec 14, 2017

Choose a reason for hiding this comment

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

not correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msohailhussain msohailhussain msohailhussain left review comments

@oakbani oakbani oakbani requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rashidsp @coveralls @msohailhussain @oakbani