fix: change conditional logic in _choose_redirect_arg for TestClent (#2783).
#2805
Conversation
starlette/testclient.py
Outdated
| @@ -449,16 +449,16 @@ def _choose_redirect_arg( | |||
| self, follow_redirects: bool | None, allow_redirects: bool | None | |||
| ) -> bool | httpx._client.UseClientDefault: | |||
| redirect: bool | httpx._client.UseClientDefault = httpx._client.USE_CLIENT_DEFAULT | |||
| if allow_redirects is not None and follow_redirects is not None: | |||
| raise RuntimeError( # pragma: no cover | |||
| if allow_redirects is not None and follow_redirects is not None: | ||
| raise RuntimeError( # pragma: no cover | ||
| "Cannot use both `allow_redirects` and `follow_redirects`." | ||
| ) | ||
| if allow_redirects is not None: | ||
| message = "The `allow_redirects` argument is deprecated. Use `follow_redirects` instead." | ||
| warnings.warn(message, DeprecationWarning) |
There was a problem hiding this comment.
When did we add this line? Maybe we should just remove this logic... 🤔
There was a problem hiding this comment.
It seems that it was added here.
I will work on the test, and if you decide that it should be removed, I will remove it.
There was a problem hiding this comment.
I saw that the allow_redirects was being covered in tests because it was being used in test_responses.py and in tests/middleware/test_https_redirect.py.
I changed it to follow_redirects as allow_redirects is deprecated and added tests to cover all the cases in test_testclient.
… and `tests/middleware/test_https_redirect.py` as it is deprecated
There was a problem hiding this comment.
I've been thinking about this...
We added the RuntimeError logic to make sure people would not use the two parameters. At the time, it was a good idea. Nowadays, it's been two years since we added this function. I think we should just drop the _choose_redirect_arg.
The deprecation warning has been there for long, so I think it's fine.
@lealre Would you like to create a PR with it?
Yes, I would like to work on it. So, just to confirm, it will remove the |
|
No, the goal is to remove the deprecated parameter. The function doesn't make sense without the parameter. |
Yes, I meant to keep the EDIT: Makes more sense to pass |
Summary
Related to issue #2783
Checklist