Skip to content

getProxy() doesn't handle proxies properly #651

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

Closed
ghost opened this issue May 7, 2019 · 7 comments
Closed

getProxy() doesn't handle proxies properly #651

ghost opened this issue May 7, 2019 · 7 comments

Comments

@ghost
Copy link

ghost commented May 7, 2019

Forum thread.

Problematic line.

return newProxy($parsed, auth)

$parsed will always return with something like:

(scheme: "http", username: "", password: "", hostname: "10.10.10.10", port: "8080", path: "", query: "", anchor: "", opaque: false)

instead of a real URL. parseUri is working with real URLs.

Choosenim is also affected..

@dom96 lets discuss the solution. I could create a MR with return newProxy(url, auth) where url could be prefixed with http:// if the scheme was empty in the condition before it.

@ghost
Copy link
Author

ghost commented May 7, 2019

We could also overload newProxy() to accept an Uri. This would also make it perform better.

@dom96
Copy link
Collaborator

dom96 commented May 7, 2019

$parsed will always return with something like: [...]

How? $ for Uri is defined in the uri module: https://nim-lang.org/docs/uri.html#%24%2CUri

@ghost
Copy link
Author

ghost commented May 7, 2019

Oh, I see! So Nim's $ strikes again(me thinking it's a virtual method)! Probably importing the whole module needs to be a Nim "good practice" because in other languages it's the opposite.

@ghost ghost closed this as completed May 7, 2019
@ghost
Copy link
Author

ghost commented May 7, 2019

In the forum thread I had from uri import parseUri.

@dom96
Copy link
Collaborator

dom96 commented May 8, 2019

oh damn. I can see the confusion, and indeed, it's subtle. Not sure what we could really do here to prevent this mistake.

@ghost
Copy link
Author

ghost commented May 8, 2019

The ones preventing this would either impact performance(vtables everywhere) or make it really uncomfortable(disabling the default $ for types in general - would need to import it). This issue affects pretty much every non-OO language so it might be a good idea to make it the linters' job.

@Araq
Copy link
Member

Araq commented May 10, 2019

@trialism We have some ideas about how to prevent this without a vtable overhead but this is not the place to discuss it, see nim-lang/RFCs#139

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants