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

Added optional callbacks to connect and disconnect #28

Closed
wants to merge 5 commits into from
Closed

Added optional callbacks to connect and disconnect #28

wants to merge 5 commits into from

Conversation

fent
Copy link
Contributor

@fent fent commented Sep 25, 2011

Some people might find this useful if they want to use callbacks instead of emitters.

connect(retryCount, callback)

disconnect(mesage, callback)

Both arguments are still optional and if the first argument is a function, it will be treated as the callback.

Oh, and I also cleaned up a bit of the code in join and part.

@martynsmith
Copy link
Owner

In principle I think this looks like a great idea (and I fully approve of the clean-up that's happened in the join/part methods).

I would like to propose a minor code prettying up though. You're doing some fairly "interesting" stuff in the connect/disconnect methods at the top to sort out the parameters. Also, it's not actually quite the same functionality now, e.g. if you specify '' (empty string) for your disconnect message then your version will use that, whereas my version would have applied the default.

Now arguably, your way might be better, but I'd rather have that sorted out in a separate (more obvious) commit if that's the case, so could we change your patch to look a little more like:

  Client.prototype.disconnect = function ( message, callback ) {
    if ( typeof(message) === 'function' ) {
        callback = message;
        message = undefined;
    }
    ...

Then the original code can pretty much be left intact below that (after binding the callback if it's a function) ?

@fent
Copy link
Contributor Author

fent commented Oct 1, 2011

I needed the check for undefined there because if the function is called without any arguments, it will set a default to retryCount/message. But checking if either of those evaluates to false works just as well and it works with an empty string too.

@fent fent closed this Oct 1, 2011
@fent fent reopened this Oct 1, 2011
@fent
Copy link
Contributor Author

fent commented Oct 10, 2011

I ran across this error. It looks like you forgot to call .toLowerCase() on a few of the commands which return the channel name. This causes an error when the channel contains any capital letters because it won't be able to find the channel object that is lowercase in the self.chans hash.

@martynsmith
Copy link
Owner

Hmmm, it seems your pull request now contains a huge number of things relating to the last pull request you did, and now this toLowerCase stuff.

Arguably toLowerCase isn't actually a good idea (depending on server implementation), so perhaps we should be removing the few cases that already exists.

If you want me to merge some of this stuff, are you able to break it up into slightly more managable chunks please? (or we could have a chat about stuff if you prefer, flick me a message) :-)

@fent
Copy link
Contributor Author

fent commented Oct 27, 2011

Sure. I'll open a pull request just for the callbacks and open up an issue for the toLowerCase problem.

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