Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Store updated channels/patterns in subscription actor #18

Merged
merged 2 commits into from
Oct 26, 2013

Conversation

ryanlecompte
Copy link
Contributor

Looks like we were just filtering and never re-assigning the result. :-)

@etaty
Copy link
Owner

etaty commented Oct 25, 2013

Good catch :)
I think we should use filterNot instead of filter
And use var instead of val for channelsSubscribed and patternsSubscribed
And put some tests haha

I let you fix your commit ;)

@ryanlecompte
Copy link
Contributor Author

Sorry about that! I just sent an update. I ended up switching the subscriber/patterns to sets to avoid any issues with duplicate subscriptions. I think it also simplified the code a bit. I also added some unit tests for this behavior.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ccf6f08 on ryanlecompte:master into 631c37f on etaty:master.

etaty added a commit that referenced this pull request Oct 26, 2013
Store updated channels/patterns in subscription actor
@etaty etaty merged commit 5eab94b into etaty:master Oct 26, 2013
@etaty
Copy link
Owner

etaty commented Oct 26, 2013

thanks

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants