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

Add clearSubscriptions method #62

Merged
merged 4 commits into from
Jul 30, 2014
Merged

Add clearSubscriptions method #62

merged 4 commits into from
Jul 30, 2014

Conversation

mroderick
Copy link
Owner

This pull request implements a clearSubscriptions method, that has been requested in #41 and #60

@mroderick
Copy link
Owner Author

@aron do you think it would be more natural to use the unsubscribe method, and not a new clearSubscriptions method?

PubSub.publishSync(topic, TestHelper.getUniqueString());

refute(spy1.called);
refute(spy2.called);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may also want to assert that it only cleared this topic, this test would also pass if all topics were cleared.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point! I think it deserves a separate test, to make it clear what's going on

@aron
Copy link
Collaborator

aron commented Jul 25, 2014

@aron do you think it would be more natural to use the unsubscribe method, and not a new clearSubscriptions method?

Now you mention it, yes that feels more natural. Instead of having the wildcard you could have a clearAllSubscriptions method. Like Node's EventEmitter has removeAllListeners.

Otherwise this looks good. I couldn't get the tests to run locally. I ran:

$ git clone <repo> && cd PubSubJS
$ npm install
$ $(npm bin)/grunt test

Running "jslint" task

Running "buster" task
>> In order for this task to work properly, Buster.JS must be installed and in
>> the system PATH (if you can run "buster" atthe command line, this task should
>> work).To install Buster.JS, run `npm install -g buster`.
Warning: Task "buster" failed. Use --force to continue.

npm install -g buster fails, I think possibly due to a rename?

@mroderick
Copy link
Owner Author

Try running the tests with npm test

@aron
Copy link
Collaborator

aron commented Jul 25, 2014

@mroderick same issue. Does it work for you from a clean install? Bear in mind I don't have any packages installed globally.

@aron
Copy link
Collaborator

aron commented Jul 25, 2014

Getting closer. Removing th $(npm bin) from the package.json helps. npm adds the bin to the path for you. Now I just need phantomjs installed. Perhaps there's buster module to install phantom for you…?

https://github.com/mroderick/PubSubJS/blob/master/package.json#L11

@mroderick
Copy link
Owner Author

@mroderick same issue. Does it work for you from a clean install? Bear in mind I don't have any packages installed globally.

It does, it also works on Travis, so I think it's another case of your funky node environment :)

@aron
Copy link
Collaborator

aron commented Jul 25, 2014

Good to know :) At least I can trust Travis.

```javascript
PubSub.clearSubscriptions('*');
// all subscriptions are removed
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs updating now.

Copy link
Owner Author

Choose a reason for hiding this comment

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

So it does. I've fixed that. Perhaps it'd be best to squash all these commits down, before merging

@aron
Copy link
Collaborator

aron commented Jul 30, 2014

🚢

aron added a commit that referenced this pull request Jul 30, 2014
@aron aron merged commit d647f47 into master Jul 30, 2014
@aron aron deleted the add-clearsubscriptions-method branch July 30, 2014 09:53
@mroderick
Copy link
Owner Author

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.

2 participants