-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
added syntax sugar to check if URI is absolute or relative #6311
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it 👍
A small detail needs to be fixed, though.
spec/std/uri_spec.cr
Outdated
@@ -54,6 +54,18 @@ describe "URI" do | |||
end | |||
end | |||
|
|||
describe ".absolute?" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"#absolute?"
spec/std/uri_spec.cr
Outdated
it { URI.parse("/foo").absolute?.should eq(false) } | ||
end | ||
|
||
describe ".relative?" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't add extensive specs for relative?
since it is just ! absolute?
. One example to ensure it generally works is enough.
The other examples should be moved to spec absolute?
to spec all variants in one place.
src/uri.cr
Outdated
@@ -144,6 +144,16 @@ class URI | |||
end | |||
end | |||
|
|||
# Returns true if URI has a scheme (e.g. http:// or https://) specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A URI is only considered absolute if it specifies a scheme and hierarchical part (authority and/or path). For example, http://#foo
is a relative URI. The implementation needs to take that into account as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious is http://#foo
valid URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also URI.parse("http://#foo").host # => ""
empty string.
Is it OK?
def absolute?
@scheme && !@host.try(&.empty?) ? true : false
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious is
http://#foo
valid URL?
The question should be, is it a valid URI. And yes, it is. Obviously, it doesn't make much sense, but serves to illustrate the issue.
@scheme && !@host.try(&.empty?)
seems to work with URI.parse
. But it looks like this depends on an implementation detail of the parser (host
is nil
if path is non-empty, but ""
if path is empty).
Example:
URI.parse("file://").absolute? # => false
URI.new("file").absolute? # => true
This should work:
@scheme && (((host = @host) && !host.empty?) || ((path = @path) && !path.empty?))
But remove ? true : false
. It doesn't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does:
true && nil
# => nil
"https://example.com" is valid absolute URL, and path
is empty.
There are too many cases to check if URL is absolute. I wouldn't like to overload the logic.
In most cases, the first variant is enought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asterite In most cases it is enough because URIs with scheme typically also include an authority or path. But that doesn't make it correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@straight-shoota Let me quote the RFC in question:
"Relative URI references are distinguished from absolute URI in that
they do not begin with a scheme name."
Going by that checking the scheme is both correct and sufficient.
But there are some additional details, see section 5.2 of https://www.ietf.org/rfc/rfc2396.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be okay than. Sorry for the confusion. There are different definitions of the same terms... but following RFC 2396 is probably the best 👍
@mamantoha Would you change it back, please?
c3d57b8
to
331fa44
Compare
@@ -144,6 +144,16 @@ class URI | |||
end | |||
end | |||
|
|||
# Returns `true` if URI has a *scheme* specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this is not enough. See #6311 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
331fa44
to
f9207b2
Compare
spec/std/uri_spec.cr
Outdated
@@ -54,6 +54,21 @@ describe "URI" do | |||
end | |||
end | |||
|
|||
describe "#absolute?" do | |||
it { URI.parse("http://www.example.com/foo").absolute?.should eq(true) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eq(true)
-> be_true
eq(false)
-> be_false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nobody cares
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's not that nobody cares, it's just that the review adds very little value. In my mind I become the author and think "Ugh, do I really have to open the computer, sit down, change that, do a commit, push, etc., just to have some code that behaves exactly the same and is in no way superior to the old code?". That's what usually drives me from continuing working on a PR (as an author).
I know be_true
is more idiomatic, but it's equivalent to eq(true)
and it can be changed later, if someone really wants to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly what I meant in longer form, thanks for clearing that up @asterite (and I apologise for my harsh tone)
f9207b2
to
0f32390
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @mamantoha 👍
No description provided.