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

Added basic psubscribe support #26

Merged
merged 4 commits into from Apr 16, 2012

Conversation

Projects
None yet
2 participants
@pmembrey
Contributor

pmembrey commented Apr 12, 2012

Hi,

I needed psubscribe support for one of my applications so I added basic support for it. It seems to work fine, and it's been running happily but I haven't done any extensive testing.

Happy to make any changes to make it compliant...

Cheers,

Pete

@knutin

This comment has been minimized.

Show comment
Hide comment
@knutin

knutin Apr 12, 2012

Collaborator

Hi Pete,

Thanks for the patch. Looks good! Would you maybe include some tests? They would be really helpful in the future to avoid breaking things.

Knut

Collaborator

knutin commented Apr 12, 2012

Hi Pete,

Thanks for the patch. Looks good! Would you maybe include some tests? They would be really helpful in the future to avoid breaking things.

Knut

Added unit tests for psubscribe and punsubscribe. Not 100% confident …
…on the punsubscribe tests though - seems to require too many ack_message() for my liking...
@pmembrey

This comment has been minimized.

Show comment
Hide comment
@pmembrey

pmembrey Apr 12, 2012

Contributor

Hi Knut,

I've added a unit test for the pattern subscribe and patter unsubscribe. I'm not confident on the unsubscribe bit as I can't figure out why I need so many ack_message(). I was adapting the previous test code, so that part of the test might need a little tweaking...

Let me know...

Cheers,

Pete

Contributor

pmembrey commented Apr 12, 2012

Hi Knut,

I've added a unit test for the pattern subscribe and patter unsubscribe. I'm not confident on the unsubscribe bit as I can't figure out why I need so many ack_message(). I was adapting the previous test code, so that part of the test might need a little tweaking...

Let me know...

Cheers,

Pete

@pmembrey

This comment has been minimized.

Show comment
Hide comment
@pmembrey

pmembrey Apr 12, 2012

Contributor

Actually, my unsubscribe tests are totally broken. That's what happens when I code at 3am with too little caffeine! ;-)

Will fix in the morning...

Contributor

pmembrey commented Apr 12, 2012

Actually, my unsubscribe tests are totally broken. That's what happens when I code at 3am with too little caffeine! ;-)

Will fix in the morning...

@pmembrey

This comment has been minimized.

Show comment
Hide comment
@pmembrey

pmembrey Apr 13, 2012

Contributor

Not so sure now... sorry to mess everyone around on this, but could someone take a look at the test for me? Happy to fix it if someone would be kind enough to point me in the right direction :)

Contributor

pmembrey commented Apr 13, 2012

Not so sure now... sorry to mess everyone around on this, but could someone take a look at the test for me? Happy to fix it if someone would be kind enough to point me in the right direction :)

@pmembrey

This comment has been minimized.

Show comment
Hide comment
@pmembrey

pmembrey Apr 16, 2012

Contributor

Can anyone give me a hand? :)

Contributor

pmembrey commented Apr 16, 2012

Can anyone give me a hand? :)

@knutin

This comment has been minimized.

Show comment
Hide comment
@knutin

knutin Apr 16, 2012

Collaborator

Sorry I didn't get back to you sooner. I had a look at the tests and found some problems. They were also happening in the other tests. I will merge this and apply my patch.

Collaborator

knutin commented Apr 16, 2012

Sorry I didn't get back to you sooner. I had a look at the tests and found some problems. They were also happening in the other tests. I will merge this and apply my patch.

knutin added a commit that referenced this pull request Apr 16, 2012

Merge pull request #26 from pmembrey/master
Added basic psubscribe support

@knutin knutin merged commit 9feabce into wooga:master Apr 16, 2012

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