-
-
Notifications
You must be signed in to change notification settings - Fork 970
Raise warning if proxy key is eg. "all" instead of "all://". #1127
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
Raise warning if proxy key is eg. "all" instead of "all://". #1127
Conversation
bf55776 to
30d3ace
Compare
|
I see the tests, should be changed, also I can add tests for the new warning appearing. @tomchristie let me know if I can continue with that |
|
Thanks for taking a go at this! So, I'd change the implementation here, for simplicity. Instead of changing anything in the Client, I'd add a single line here... Line 481 in 78cf16a
raising a warning such as f'Proxy keys should use proper URL forms rather than plain scheme strings. Instead of "{pattern}", use "{pattern}://"'.
That ought to be enough to deal with this more neatly. You probably also want to then update the docs in https://github.com/encode/httpx/blob/master/docs/advanced.md to use the preferred styles. |
93d3032 to
df74492
Compare
…ies={"http": ...} instead of {"http://": ...}. Updated docs and added unit, which check the warning presence
df74492 to
592e344
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.
Awesome!
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.
Meant to approve in advance. :-)
Co-authored-by: Florimond Manca <[email protected]>
Closes #1105