Switch follow redirects default #1808
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I think I like the "explicit better than implicit" vibe to this. At the same time I wonder about usability. But isn't there precedent to this anyway? Doesn't cURL ignore redirects and return 3xx by default too? I think so? |
| @@ -34,7 +34,7 @@ def request( | |||
| auth: AuthTypes = None, | |||
| proxies: ProxiesTypes = None, | |||
| timeout: TimeoutTypes = DEFAULT_TIMEOUT_CONFIG, | |||
| allow_redirects: bool = True, | |||
| follow_redirects: bool = False, | |||
There was a problem hiding this comment.
I assume we don't consider a soft deprecation (with a warning) to be worth it here, given the amount of code that would require?
|
This feels potentially nice, but we need to be careful. Eg. Potentially it changes what expectations you might want on r = httpx.get(...)
r.raise_for_status() # If 3xx responses *aren't* handled by default, then you might want this to raise exceptions for 3xx cases.
data = r.json()Urg. Awkward conflicted imperfect design decisions. Ugh. |
2 tasks
bdraco
added a commit
to bdraco/home-assistant
that referenced
this pull request
Dec 8, 2021
- This is the current behavior in 2021.11 but changed due to a breaking change in httpx see encode/httpx#1808 - Fixes home-assistant#61239
22 tasks
ikhoon
pushed a commit
to line/centraldogma-python
that referenced
this pull request
Dec 31, 2021
Motivation -- - There is [a breaking change](encode/httpx#1808) replacing `allow_redirects` with `follow_redirects` in `httpx`. - Without changing our test case, it is possible that developers/users can be confused by our code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implementation based on discussion #1785.
Switches the auto-redirect behaviour to off by default.
The benefit of this change would be to make it more explicit when redirects should be followed, and avoid code that unintentionally issues multiple requests, as a result of not requesting the correct URL.
We do still allow auto-redirects, but only if explicitly enabled...
Or, by enabling a client-wide default...
Also note that we change
allow_redirectstofollow_redirects.This is a big change, but I think it may well be one of those points where it's worth us diverging from requests.
It'd also play more neatly into the behaviour that we'd probably want by default from a command-line client, which I'd really like to see on our roadmap.