Skip to content

Raise warning if proxy key is eg. "all" instead of "all://". - #1127

Merged
lovelydinosaur merged 5 commits into
encode:masterfrom
cdeler:raise_warning_if_proxy_scheme_is_old_fashioned
Aug 5, 2020
Merged

Raise warning if proxy key is eg. "all" instead of "all://".#1127
lovelydinosaur merged 5 commits into
encode:masterfrom
cdeler:raise_warning_if_proxy_scheme_is_old_fashioned

Conversation

@cdeler

@cdelercdeler commented Aug 4, 2020

Copy link
Copy Markdown
Contributor

Closes #1105

@cdeler
cdelerforce-pushed the raise_warning_if_proxy_scheme_is_old_fashioned branch from bf55776 to 30d3aceCompareAugust 4, 2020 19:20
@cdeler

Copy link
Copy Markdown
ContributorAuthor

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

lovelydinosaur commented Aug 5, 2020

Copy link
Copy Markdown
Contributor

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.

@lovelydinosaurlovelydinosaur 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
@lovelydinosaurlovelydinosaur 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
@lovelydinosaurlovelydinosaur added the enhancement New feature or request label Aug 5, 2020
@cdeler
cdelerforce-pushed the raise_warning_if_proxy_scheme_is_old_fashioned branch from 93d3032 to df74492CompareAugust 5, 2020 14:50
…http": ...} instead of {"http://": ...}. Updated docs and added unit, which check the warning presence
@cdeler
cdelerforce-pushed the raise_warning_if_proxy_scheme_is_old_fashioned branch from df74492 to 592e344CompareAugust 5, 2020 14:57

@florimondmancaflorimondmanca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome!

Comment threadtests/client/test_proxies.py Outdated

@florimondmancaflorimondmanca left a comment

Copy link
Copy Markdown
Contributor

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 <florimond.manca@gmail.com>
Comment threadtests/client/test_proxies.py Outdated
@lovelydinosaur
lovelydinosaur merged commit 7279ed4 into encode:masterAug 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
@lovelydinosaurlovelydinosaur 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Development

Successfully merging this pull request may close these issues.

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

3 participants

@cdeler@lovelydinosaur@florimondmanca