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

Subscribe doesn't register listener if no callback #282

Merged
merged 2 commits into from
Jun 16, 2014

Conversation

gabriel
Copy link
Contributor

@gabriel gabriel commented Feb 25, 2014

If you call @faye.subscribe("/channel/**") without a callback (block), then it doesn't get added to @channels[name], so unsubscribe doesn't work. This was the problem I was having yesterday. Sometimes I did use a block and sometimes not, which confused me.

Let me know if I can be more help here. I can write a spec but the way the specs are setup subscribe is always taking a block so...

@jcoglan
Copy link
Collaborator

jcoglan commented Feb 26, 2014

I don't understand what problem this is supposed to fix. Can you provide an example, or write a spec that covers the change?

I don't think this would have any functional effect at all; all it does is that instead of returning early if callback.nil?, it loops over the names and does nothing on each iteration.

@gabriel
Copy link
Contributor Author

gabriel commented Feb 26, 2014

Yeah I'll write a spec...

Look closer, the loop is setting @channels[name] ||= ..

On Wednesday, February 26, 2014, James Coglan notifications@github.com
wrote:

I don't understand what problem this is supposed to fix. Can you provide
an example, or write a spec that covers the change?

I don't think this would have any functional effect at all; all it does is
that instead of returning early if callback.nil?, it loops over the names
and does nothing on each iteration.

Reply to this email directly or view it on GitHubhttps://github.com//pull/282#issuecomment-36117079
.

@jcoglan
Copy link
Collaborator

jcoglan commented Mar 2, 2014

The spec is a unit test for the behaviour you've added, but I still don't understand why it's needed. What problem is the Client exhibiting that necessitates this change?

@jcoglan jcoglan merged commit 52b4494 into faye:master Jun 16, 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.

2 participants