Skip to content

Conversation

@cdeler
Copy link
Contributor

@cdeler cdeler commented Aug 4, 2020

Closes #1105

@cdeler cdeler force-pushed the raise_warning_if_proxy_scheme_is_old_fashioned branch from bf55776 to 30d3ace Compare August 4, 2020 19:20
@cdeler
Copy link
Contributor Author

cdeler commented Aug 4, 2020

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

@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Aug 5, 2020

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...

pattern += "://"

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.

@lovelydinosaur lovelydinosaur changed the title WIP: #1105 raise warning if proxy scheme is http, https, all instead of http://, https://, all:// Raise warning if proxy scheme is http, https, all instead of http://, https://, all:// Aug 5, 2020
@lovelydinosaur lovelydinosaur changed the title Raise warning if proxy scheme is http, https, all instead of http://, https://, all:// Raise warning if proxy key is eg. "all" instead of "all://". Aug 5, 2020
@lovelydinosaur lovelydinosaur added the enhancement New feature or request label Aug 5, 2020
@cdeler cdeler force-pushed the raise_warning_if_proxy_scheme_is_old_fashioned branch from 93d3032 to df74492 Compare August 5, 2020 14:50
…ies={"http": ...} instead of {"http://": ...}. Updated docs and added unit, which check the warning presence
@cdeler cdeler force-pushed the raise_warning_if_proxy_scheme_is_old_fashioned branch from df74492 to 592e344 Compare August 5, 2020 14:57
Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

Copy link
Contributor

@florimondmanca florimondmanca left a 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. :-)

@lovelydinosaur lovelydinosaur merged commit 7279ed4 into encode:master Aug 5, 2020
cdeler added a commit to cdeler/httpx that referenced this pull request Aug 6, 2020
cdeler added a commit to cdeler/httpx that referenced this pull request Aug 6, 2020
@lovelydinosaur lovelydinosaur mentioned this pull request Aug 6, 2020
@cdeler cdeler deleted the raise_warning_if_proxy_scheme_is_old_fashioned branch September 9, 2020 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

On proxies={"http": ...} vs. proxies={"http://": ...}

3 participants