Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rework host/hostname/authority implementation. #1561
Rework host/hostname/authority implementation. #1561
Changes from 6 commits
488dae7
26e4d7c
b9b87a7
7e899c8
66ea112
aa68f07
dc44973
a146b6a
eb7de84
eec2635
05212cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If we state that
SERVER_PORT must be an integer
, we should doenv["SERVER_PORT"].is_a?(Integer)
. I don't think we need to check that it is convertable to an integer, unless we want that to be the spec.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.
Okay, makes sense, I'll update the SPEC.
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.
In #1557 (comment), you stated
I'm fine with implicit return when the cyclomatic complexity of a function is 1, or the branch depth is 1.
To keep at least a reasonably consistent style, can we avoid explicit returns in cases where the branch depth is <= 1 (I assume the branch depth is 1 here, and outside of the if the branch depth is 0). Ditto for the rest of the file.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.
As
SERVER_PORT
is required, we don't need the if. Also, if we change SPEC to require it be an integer, we don't need to convert it, so this method could just beget_header(SERVER_PORT)
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.
Unused local variables, method body could probably just be
split_authority(authority)[0]
. Likewisehostname
could besplit_authority(authority)[1]
. If you want really keep the existing style, use underscores for the unused local variables. Similar issue exists in theport
method below.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.
This looks like inconsistency: the method
hostname
returnsaddress
… why? I think we should name it eitherhostname
oraddress
ecerywhere.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 matches the implementation of
URI
. I'm not saying it's right. I just decided to follow the existing design/method names. But you are right. Maybe it should beaddress
or something else...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 mean, if you even just rename local variable to
hostname
— it'd be better. I don't seeaddress
as URI method or in its sources.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.
Understood. It is actually confusing to me too in a way.
What does make sense to me is when you say "host_with_port". That means, the "escaped" host and
:port
appended. You can't havehostname_with_port
. It's simply not possible because it becomes ambiguous to parse.So, there's that logic... But yeah, I also see it's confusing.
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.
Yes, we could rename it
hostname
everywhere. I see what you are saying, and I don't think it would be a problem. Except that technically, I don't even know if192.168.1.1
is a hostname? Or1::
is a hostname? To me, something likewww.google.com
is a hostname. Not sure what is the right word/definition here.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 know it's about local variable name but the entire thing is kind of confusing, so we should try to get it right for our users or we just perpetuate the chaos.
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.
So we have several options:
1/ Rename local variable
address
->hostname
andreturn address
toreturn hostname
.2/ Rename method
def hostname
todef address
.3/ Remove
def hostname
.4/ Add
def uri
which returns an instance of URI which represents what the URI was used to make the request.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.
1 is like compliance with URI from stdlib.
2 is like try to improve naming, but, as I see, still with logic violations.
3 is radical, and, I guess, we still need for a helper method. If not — the best code is no code.
4 seems interesting, and "URI" sounds common enough for IPs and domains, especially with
URI
instance inside.So, I can't say before trying, but I'd try to make in this order: 3, 4, 1, 2.
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'll wait for feedback from @jeremyevans before I make any further changes.
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 for the record, there are several new methods, and that includes
def hostname
. It's not released in Rack 2.1.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.
If you are going to set the
scheme
local variable, you should use it as the argument toDEFAULT_PORTS.[]
. Otherwise don't define the local variable.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 generic name like
split_header
implies it will split all headers, which I don't think is possible as headers have different formats. Maybesplit_ip_addresses
wasn't accurate, but I thinksplit_header
is more misleading as a name.