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

Fixed bug when sending empty strings or several lines to say #21

Merged
merged 4 commits into from
Sep 4, 2011
Merged

Conversation

eirikb
Copy link
Contributor

@eirikb eirikb commented Aug 17, 2011

This will in theory make it possible to send several lines and empty lines to say, like this:

client.say('#test', '');

and

client.say('#test', 'Hello!\nworld');

Note that it is not widely tested

@martynsmith
Copy link
Owner

Just looking, shouldn't the line ending regexp be more like /\r?\n/ ? (that caters for DOS and Unix line endings, what do Macs do?)

@eirikb
Copy link
Contributor Author

eirikb commented Sep 3, 2011

True! Good one.
What we could do is this:

.split(/\r|\r?\n/)

Then we should get \r\n, \r and \n

btw, I have been using this code (with updates) since I made the pull request and it has not given me any trouble so far. I also added a max-lines-count so the channel would not get spammed, but that would be a config-thing so not sure how to add it to the library

@martynsmith
Copy link
Owner

Yeah, I'm just curious though, does anyone ever use \r by itself? I didn't
think they did, but ultimately yeah, that looks reasonable :-)

Are you able to mash that into a new pull request (or perhaps in to the
existing one) ?

On Sat, Sep 3, 2011 at 7:56 PM, eirikb <
[email protected]>wrote:

True! Good one.
What we could do is this:

.split(/\r|\r?\n/)

Then we should get \r\n, \r and \n

btw, I have been using this code (with updates) since I made the pull
request and it has not given me any trouble so far. I also added a
max-lines-count so the channel would not get spammed, but that would be a
config-thing so not sure how to add it to the library

Reply to this email directly or view it on GitHub:
#21 (comment)

@eirikb
Copy link
Contributor Author

eirikb commented Sep 3, 2011

I made a commit with your suggestion, it will probably cover all cases :)

@martynsmith
Copy link
Owner

Sweet, now for some reason this can't be automatically merged, so err, I'll try sort it out sometime soon :-)

@martynsmith martynsmith merged commit 7d8074a into martynsmith:master Sep 4, 2011
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