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

Split the username and password correctly #330

Merged
merged 1 commit into from
Mar 7, 2017

Conversation

dietervanhoof
Copy link
Contributor

@dietervanhoof dietervanhoof commented Feb 23, 2017

Split the username and password correctly so that the password can contain a ':' character.

Split the username and password correctly so that the password can contain ':' character.
@squaremo
Copy link
Collaborator

Oops, that's a good spot. Instead of the split on regexp, which I must admit I didn't grok on first scan, would it be better to use indexOf, wdyt?

@dietervanhoof
Copy link
Contributor Author

dietervanhoof commented Feb 28, 2017

Apparently indexOf would be a tiny bit faster when using smaller strings compared to a regex. I wouldn't mind it using indexOf but I feel like this code is a lot cleaner than something like

user = parts.auth.substring(0, parts.auth.indexOf(':'));
pass = parts.auth.substring(parts.auth.indexOf(':') + 1, parts.auth.length)

It's all up to you.

@squaremo squaremo merged commit 615f870 into amqp-node:master Mar 7, 2017
@squaremo
Copy link
Collaborator

squaremo commented Mar 7, 2017

I did end up rewriting it around indexOf(':'), mainly because there's a special case (no colon) that I wanted to be explicit.

Thanks for this PR, it fixed the problem but it also made me rethink some of the surrounding bits, revisit the specs, and add some tests!

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.

None yet

2 participants