ureq
ureq copied to clipboard
Add Response::remote_addr()
Fixes #488.
Sorry for the delay, I've finally found time to look into this again.
I've pushed a new implementation that doesn't use TcpStream::peer_addr()
and where Response::remote_addr()
returns a SocketAddr
instead of an Option<SocketAddr>
.
I'm still unsure about two things regarding this new implementation:
- I'm not familiar with SOCKS proxies. I think with this implementation,
Response::remote_addr()
returns the socket address of the proxy. I'm not sure if this is the wanted behaviour; if not, I would need a few more pointers on how to change it so that the actual server's address is returned. Either way, this should probably be documented, e.g. by adding "When using a SOCKS proxy, this returns the proxy's address." toResponse::remote_addr()
's documentation. - When using
Stream::from_vec()
, it is not clear what the remote address should be. If I understand correctly, this method is only used for tests and forResponse::from_str()
. In tests, the remote address isn't used;Response::from_str()
already hard-codesexample.com
as the request URL, so I took the liberty to hard-code the remote address to0.0.0.0:0
. Please let me know if you disagree with this.
Whoops, I have to admit I forgot about this. 😅
I'd be happy to rebase and resolve the conflicts, but before diving back into ureq
's code, it'd be great if you could give me some feedback regarding the current implementation and my two concerns mentioned in the last comment.
Glad to hear that! :) I've merged the upstream changes, please take a final look.
Would you mind rebasing off the latest main, so we can see a clean test run?
Done
Alright, now that I ran cargo test
with all the features enabled all the tests should compile and pass 😅
There's also the test.sh
bash script that runs a few of the feature flag combos the CI runs.
Good to know!
@mvforell There are some conflicts against main.
@mvforell There are some conflicts against main.
Hm, GitHub says "This branch has no conflicts with the base branch", and my local checkout says:
> git fetch upstream && git merge upstream/main
Already up to date.
@mvforell sorry! must have had an old browser tab or something.
I'm really happy to see this merged, thank you for your feedback and pointers!
Do you have an estimate for when the next version including this will be released?