Skip to content

Conversation

@dkubb
Copy link

@dkubb dkubb commented May 6, 2013

This branch refactors the URL generation to use URI to construct the string, rather than using string concatenation directly.

It also cleans up a few small build issues I encountered while working with the gem in development.

@pat
Copy link

pat commented May 6, 2013

Certainly feels cleaner. Would be great to see this merged in.

@bartt
Copy link
Owner

bartt commented May 6, 2013

Splitting URL strings on characters is never a good idea. Use URI instead.

@dkubb
Copy link
Author

dkubb commented May 7, 2013

It's not splitting a URL, it's splitting a hostname that is a host with an optional port. afaik there is no URI method to parse/split hostname. To use URI you'd have to use string concatenation to build a URL string first then split it using something like URI.split, which kind of defeats the purpose of this change and is (IMHO) worse than splitting a "host with port" in the way this code does now.

If I'm wrong and there is a URI method that you can hand a hostname to, let please me know and I'll update the PR.

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.

3 participants