#99 - add --resolve option #659
Conversation
httpie/resolve.py
Outdated
| self.entries = {} | ||
| for entry in resolve: | ||
| host, port_and_addresses = entry.split(':', 1) | ||
| if ':' in port_and_addresses: |
There was a problem hiding this comment.
This logic is not very clear. It could use some commenting I think.possibly to show possible examples.
# ho.st:1.2.3.4
if ...
# ho.st:1.2.3.4:[::1]
if ...
There was a problem hiding this comment.
For this and the next comment, would something like:
@staticmethod
def parse_resolve_entry(entry):
"""
:param entry:
:return: host, port (or `None` if absent) and
"""
host, port_and_addresses = entry.split(':', 1)
try:
# If port is not absent, e.g. 'example.org:80:127.0.0.1'
port, addresses = port_and_addresses.split(':', 1)
port = int(port)
except ValueError:
port = None
addresses = port_and_addresses
return host, port, [
a[1:-1] if a.startswith('[') and a.endswith(']') else a
for a in addresses.split(',')
]
def __init__(self, resolve):
"""
:param resolve: A list of HOST[:PORT]:ADDRESS[:ADDRESS]? records
:type resolve: list[str]
"""
self.entries = {
(host, port): addresses
for host, port, addresses in map(self.parse_resolve_entry, resolve)
}
work to make the logic more clear?
There was a problem hiding this comment.
There's no need/benefit to having this be a static method over a global function. Other than that, I think it makes sense.
There was a problem hiding this comment.
Separate function is for splitting what's going inside of __init__ into actual parsing (the function itself) and the internal state of the object. Otherwise, __init__ is violating the principle of a single responsibility.
There was a problem hiding this comment.
I was saying there is no need for parse_resolve_entry to be a stactic method. It can be a global function. That is what I was saying.
def _parse_resolve_entry(...):
# ...
class CustomResolver:Static methods are usually used as part of the interface to the class, but it's unexpected for anyone to be calling CustomResolver.parse_resolve_entry(...).
httpie/resolve.py
Outdated
| if address[:1] == '[' and address[-1:] == ']': | ||
| address = address[1:-1] | ||
| final_addresses.append(address) | ||
| self.entries[(host, port)] = final_addresses |
There was a problem hiding this comment.
self.entries[(host, port)] = list(map(
lambda a: a[1:-1] if a[:1] == '[' and a[-1:] == ']' else a,
addresses.split(',')
))
httpie/resolve.py
Outdated
| class CustomResolver: | ||
| def __init__(self, resolve): | ||
| """ | ||
| :param resolve: A list of HOST[:PORT]:ADDRESS[:ADDRESS]? records |
There was a problem hiding this comment.
This comment is not very clear. It looks like there can possibly be 4 pieces from record.split(":").
There was a problem hiding this comment.
Could you suggest a better message in order to show there could be one or more trailing address? Would :ADDRESS[:ADDRESS...] work?
There was a problem hiding this comment.
Can you show me a real world example of this usage? I'm not sure I understand how it will look and function based on the code.
There was a problem hiding this comment.
Sure. For example localhost:[::1],[::2],[::3] would resolve localhost into a list of addresses: ::1, ::2, ::3 which will make httpie to try to connect to all 3 of them one-by-one.
…cting parsing function
|
@jakubroztocil is there any chance to merge this? Let me know if anything needs to be improved! |
|
This functionality would be great to have working with httpie! |
|
Any update in 2020? |
|
Thank you very much for the effort and for the proposed patch and sorry to leave you hanging for such a long time. The reason for it was our uncertainty about how to approach this feature. After talking through the pros and cons, the strong preference here would be to fix the underlying issue in urllib3 (see urllib3/urllib3#1209 ). Monkeypatching often leads to unintended side-effects and interactions, especially when done on system libraries. HTTPie is used in wide variety of systems and configurations and we'd strongly prefer not to expose this risk. That said, if we'll end up going down the monkeypatch route, we'll definitely take this as an inspiration. Any help with work on the urllib3 patch is appreciated. Otherwise, we'll give it a try, but let's see how this ends up priority-wise against other features. See #99 for more context. Also, if you'd like to discuss more on how to approach this, feel free to join our Discord! Again, thanks for the time and effort. It's really appreciated. |
Added
--resolveoption. Few points I'd like to get help with:CustomResolverbeing created in thecore.py:program?README.rst?--resolve example.org:::1,::2would also work. Is that fine or square brackets syntax should be enforced so it would be mandatory to write something like--resolve example.org:[::1],[::2]?