Use URI to build URIs and hostname= to wrap ipv6 addresses in brackets. #16
Conversation
|
@Fryguy @brandondunne Please review. Related: rest-client/rest-client#332 |
|
Looks good, will merge when green. |
| end | ||
| require 'uri' | ||
| uri = URI::Generic.build(:scheme => scheme.to_s, :port => port) | ||
| uri.hostname = server |
There was a problem hiding this comment.
@jrafanie Any reason not to add :host to the line above?
There was a problem hiding this comment.
Then we could avoid the variable assignment and just do:
def base_uri
require 'uri'
URI::Generic.build(:scheme => scheme.to_s, :host => server, :port => port).to_s
endThere was a problem hiding this comment.
Never mind... Now I see that #hostname= handles IPv6
There was a problem hiding this comment.
@brandondunne good call, I'm actually going to open a PR on ruby to fix build so I can pass :host => "::1" and have it run hostname= under the covers so it handles "bare" ipv6 addresses. Once that is fixed in upstream, this code can be simplified as you suggest.
There was a problem hiding this comment.
@jrafanie Is there any disadvantage to that? I guess worst case, it could be opened as allowing a :hostname key so that they match the attribute methods, right?
There was a problem hiding this comment.
There is no disadvantage. Clients shouldn't need to know that ::1 needs to become http://[::1] in URIs. We'll see what they say. For now, this workaround is good enough.
|
Looks Good 🍰 |
…to_build_uris Use URI to build URIs and hostname= to wrap ipv6 addresses in brackets.
See: https://bugs.ruby-lang.org/issues/3788
Related: https://www.ietf.org/rfc/rfc2732.txt