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

more rubyish #482

Merged
merged 1 commit into from
Dec 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 12 additions & 11 deletions lib/addressable/uri.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def self.parse(uri)
uri = uri.to_str
rescue TypeError, NoMethodError
raise TypeError, "Can't convert #{uri.class} into String."
end if not uri.is_a? String
end unless uri.is_a?(String)

# This Regexp supplied as an example in RFC 3986, and it works great.
scan = uri.scan(URIREGEX)
Expand All @@ -138,15 +138,15 @@ def self.parse(uri)
user = userinfo.strip[/^([^:]*):?/, 1]
password = userinfo.strip[/:(.*)$/, 1]
end

host = authority.sub(
/^([^\[\]]*)@/, EMPTY_STR
).sub(
/:([^:@\[\]]*?)$/, EMPTY_STR
)

port = authority[/:([^:@\[\]]*?)$/, 1]
end
if port == EMPTY_STR
port = nil
port = nil if port == EMPTY_STR
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this assignment was moved inside if authority != nil where it wasn't before.
Not sure it matters (all tests still pass, but we might lack coverage).

Spotted this when looking at the files instead of the diff.

This PR

if authority != nil
# The Regexp above doesn't split apart the authority.
userinfo = authority[/^([^\[\]]*)@/, 1]
if userinfo != nil
user = userinfo.strip[/^([^:]*):?/, 1]
password = userinfo.strip[/:(.*)$/, 1]
end
host = authority.sub(
/^([^\[\]]*)@/, EMPTY_STR
).sub(
/:([^:@\[\]]*?)$/, EMPTY_STR
)
port = authority[/:([^:@\[\]]*?)$/, 1]
port = nil if port == EMPTY_STR
end

main

if authority != nil
# The Regexp above doesn't split apart the authority.
userinfo = authority[/^([^\[\]]*)@/, 1]
if userinfo != nil
user = userinfo.strip[/^([^:]*):?/, 1]
password = userinfo.strip[/:(.*)$/, 1]
end
host = authority.sub(
/^([^\[\]]*)@/, EMPTY_STR
).sub(
/:([^:@\[\]]*?)$/, EMPTY_STR
)
port = authority[/:([^:@\[\]]*?)$/, 1]
end
if port == EMPTY_STR
port = nil
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct. port is set to nil on line 133 before the block, so this check only needs to happen if the authority block changes the port value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test to ensure proper functionality

end

return new(
Expand Down Expand Up @@ -189,7 +189,7 @@ def self.heuristic_parse(uri, hints={})
uri = uri.to_s
end

if !uri.respond_to?(:to_str)
unless uri.respond_to?(:to_str)
raise TypeError, "Can't convert #{uri.class} into String."
end
# Otherwise, convert to a String
Expand Down Expand Up @@ -281,7 +281,7 @@ def self.convert_path(path)
return nil unless path
# If a URI object is passed, just return itself.
return path if path.kind_of?(self)
if !path.respond_to?(:to_str)
unless path.respond_to?(:to_str)
raise TypeError, "Can't convert #{path.class} into String."
end
# Otherwise, convert to a String
Expand Down Expand Up @@ -329,13 +329,13 @@ def self.convert_path(path)
# #=> #<Addressable::URI:0xcab390 URI:http://example.com/relative/path>
def self.join(*uris)
uri_objects = uris.collect do |uri|
if !uri.respond_to?(:to_str)
unless uri.respond_to?(:to_str)
raise TypeError, "Can't convert #{uri.class} into String."
end
uri.kind_of?(self) ? uri : self.parse(uri.to_str)
end
result = uri_objects.shift.dup
for uri in uri_objects
uri_objects.each do |uri|
result.join!(uri)
end
return result
Expand Down Expand Up @@ -838,7 +838,7 @@ def initialize(options={})

reset_ivs

self.defer_validation do
defer_validation do
# Bunch of crazy logic required because of the composite components
# like userinfo and authority.
self.scheme = options[:scheme] if options[:scheme]
Expand All @@ -853,7 +853,8 @@ def initialize(options={})
self.query_values = options[:query_values] if options[:query_values]
self.fragment = options[:fragment] if options[:fragment]
end
self.to_s

to_s # force path validation
end

##
Expand Down Expand Up @@ -1998,7 +1999,7 @@ def join!(uri)
#
# @see Hash#merge
def merge(hash)
if !hash.respond_to?(:to_hash)
unless hash.respond_to?(:to_hash)
raise TypeError, "Can't convert #{hash.class} into Hash."
end
hash = hash.to_hash
Expand Down
14 changes: 14 additions & 0 deletions spec/addressable/uri_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3021,6 +3021,20 @@ def to_s
end
end

describe Addressable::URI, "when parsed with empty port" do
subject(:uri) do
Addressable::URI.parse("//example.com:")
end

it "should not infer a port" do
expect(uri.port).to be(nil)
end

it "should have a site value of '//example.com'" do
expect(uri.site).to eq("//example.com")
end
end

describe Addressable::URI, "when parsed from " +
"'http://example.com/%2E/'" do
before do
Expand Down