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

Fix data split delimiter #280

Merged
2 commits merged into from
Jan 10, 2015
Merged

Conversation

ota42y
Copy link
Contributor

@ota42y ota42y commented Jan 7, 2015

IRC Protocol use CR-LF, but legacy IRC server send other code.
http://tools.ietf.org/html/rfc1459.html#section-8

So, replace split string from \r\n with /\r\n|\r|\n/

@jirwin
Copy link
Collaborator

jirwin commented Jan 7, 2015

+1

@ghost
Copy link

ghost commented Jan 8, 2015

I'm a little wary of the precedence of the | in this regex and I want to torture-test it a bit before merging.

@ghost
Copy link

ghost commented Jan 8, 2015

I do not think this regex works in full generality. Consider the following:

var string = 'hello\nthere\nworld\r\nstuff'
var badRe = new RegExp('/\r\n|\r|\n/')
console.dir(string.split(badRe));
var goodRe = new RegExp('(?:\r\n)|\n|\r')
console.dir(string.split(goodRe));

Can you consider the second regex in this example for your pull req, or point out where I've misunderstood? Thanks!

@jirwin
Copy link
Collaborator

jirwin commented Jan 8, 2015

@apeiron Great catch. We may also want to consider compiling the regex out of scope of the listener so it doesn't have to be created for every chunk of data coming in.

@ota42y
Copy link
Contributor Author

ota42y commented Jan 9, 2015

@jirwin Ok, I moved the regex.

@apeiron thanks! I overlooked that's pattern.

But, I think you should delete / word from the badRe, and it's worked well.

var string = 'hello\nthere\nworld\r\nstuff'
var badRe = new RegExp('\r\n|\r|\n')
var badResult = string.split(badRe)
console.dir(badResult);
var goodRe = new RegExp('(?:\r\n)|\n|\r')
var goodResult = string.split(goodRe);
console.dir(goodResult)

var isEqual = (badResult.length == goodResult.length)

for (var i = 0; i < badResult.length; ++i) {
 if (badResult[i] !== goodResult[i]){
   isEqual = false
 }
}

console.dir(isEqual) # true

I think /\r\n|\r|\n/ isn't equal to\r\n|\r|\n in RegExp.

Probably, the RegExp object interpret it as \/\r\n|\r|\n\/.
Because, the result executing string.split(/\/\r\n|\r|\n\//) is equal to executing string.split(new RegExp('/\r\n|\r|\n/')).

So I fixed code using new RegExp('\r\n|\r|\n')

@ghost
Copy link

ghost commented Jan 9, 2015

This needs rebasing, because we just merged #278.

@ghost ghost added the needs-rebase label Jan 9, 2015
@ota42y ota42y force-pushed the hotfix/delimiter_fix branch from d199bdf to 84c2110 Compare January 10, 2015 02:22
@ota42y
Copy link
Contributor Author

ota42y commented Jan 10, 2015

I rebased.

@jirwin
Copy link
Collaborator

jirwin commented Jan 10, 2015

+1

ghost pushed a commit that referenced this pull request Jan 10, 2015
@ghost ghost merged commit c98030f into martynsmith:master Jan 10, 2015
@ota42y ota42y deleted the hotfix/delimiter_fix branch January 10, 2015 12:28
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants