Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove deprecated Retry options #2086

Merged
merged 2 commits into from
Nov 24, 2020
Merged

Remove deprecated Retry options #2086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4feeca4
Remove mentions of whitelist in tests
pquentin Nov 24, 2020
ba59347
Remove deprecated Retry options
pquentin Nov 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
120 changes: 5 additions & 115 deletions src/urllib3/util/retry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import logging
import re
import time
import warnings
from collections import namedtuple
from itertools import takewhile

Expand All @@ -26,49 +25,7 @@
)


# TODO: In v2 we can remove this sentinel and metaclass with deprecated options.
_Default = object()


class _RetryMeta(type):
@property
def DEFAULT_METHOD_WHITELIST(cls):
warnings.warn(
"Using 'Retry.DEFAULT_METHOD_WHITELIST' is deprecated and "
"will be removed in v2.0. Use 'Retry.DEFAULT_METHODS_ALLOWED' instead",
DeprecationWarning,
)
Comment on lines -36 to -40
Copy link

Choose a reason for hiding this comment

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

FYI, this deprecation warning hadn’t been visible, because it was emitted without a stacklevel and therefore filtered by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

@andersk I'm sorry that you haven't seen the warning, but I don't think stacklevel has anything to do about it? It just changes the way the stack trace is displayed

Copy link

@andersk andersk Jan 14, 2022 •
edited

Choose a reason for hiding this comment

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

It changes where Python thinks the warning originated, which changes whether it will be filtered. For example, this only shows a deprecation warning for g:

$ cat a.py
import b
b.f()
b.g()

$ cat b.py
import warnings
def f():
    warnings.warn("f", DeprecationWarning)
def g():
    warnings.warn("g", DeprecationWarning, stacklevel=2)

$ python3 a.py
/tmp/a.py:3: DeprecationWarning: g
  b.g()

This is called out in the documentation I linked above: “This [stacklevel=2] makes the warning refer to deprecation()’s caller, rather than to the source of deprecation() itself (since the latter would defeat the purpose of the warning message).”

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, wow, thanks for taking the time to educate us, I had not realized "defeating the purpose" meant "you won't see it". Will keep this in mind for any future warnings we add!

Sorry about that

return cls.DEFAULT_ALLOWED_METHODS

@DEFAULT_METHOD_WHITELIST.setter
def DEFAULT_METHOD_WHITELIST(cls, value):
warnings.warn(
"Using 'Retry.DEFAULT_METHOD_WHITELIST' is deprecated and "
"will be removed in v2.0. Use 'Retry.DEFAULT_ALLOWED_METHODS' instead",
DeprecationWarning,
)
cls.DEFAULT_ALLOWED_METHODS = value

@property
def DEFAULT_REDIRECT_HEADERS_BLACKLIST(cls):
warnings.warn(
"Using 'Retry.DEFAULT_REDIRECT_HEADERS_BLACKLIST' is deprecated and "
"will be removed in v2.0. Use 'Retry.DEFAULT_REMOVE_HEADERS_ON_REDIRECT' instead",
DeprecationWarning,
)
return cls.DEFAULT_REMOVE_HEADERS_ON_REDIRECT

@DEFAULT_REDIRECT_HEADERS_BLACKLIST.setter
def DEFAULT_REDIRECT_HEADERS_BLACKLIST(cls, value):
warnings.warn(
"Using 'Retry.DEFAULT_REDIRECT_HEADERS_BLACKLIST' is deprecated and "
"will be removed in v2.0. Use 'Retry.DEFAULT_REMOVE_HEADERS_ON_REDIRECT' instead",
DeprecationWarning,
)
cls.DEFAULT_REMOVE_HEADERS_ON_REDIRECT = value


class Retry(metaclass=_RetryMeta):
class Retry:
"""Retry configuration.

Each retry attempt will create a new Retry object with updated values, so
Expand Down Expand Up @@ -157,11 +114,6 @@ class Retry(metaclass=_RetryMeta):

Set to a ``False`` value to retry on any verb.

.. warning::

Previously this parameter was named ``method_whitelist``, that
usage is deprecated in v1.26.0 and will be removed in v2.0.

:param iterable status_forcelist:
A set of integer HTTP status codes that we should force a retry on.
A retry is initiated if the request method is in ``allowed_methods``
Expand Down Expand Up @@ -227,36 +179,15 @@ def __init__(
redirect=None,
status=None,
other=None,
allowed_methods=_Default,
allowed_methods=DEFAULT_ALLOWED_METHODS,
status_forcelist=None,
backoff_factor=0,
raise_on_redirect=True,
raise_on_status=True,
history=None,
respect_retry_after_header=True,
remove_headers_on_redirect=_Default,
# TODO: Deprecated, remove in v2.0
method_whitelist=_Default,
remove_headers_on_redirect=DEFAULT_REMOVE_HEADERS_ON_REDIRECT,
):

if method_whitelist is not _Default:
if allowed_methods is not _Default:
raise ValueError(
"Using both 'allowed_methods' and "
"'method_whitelist' together is not allowed. "
"Instead only use 'allowed_methods'"
)
warnings.warn(
"Using 'method_whitelist' with Retry is deprecated and "
"will be removed in v2.0. Use 'allowed_methods' instead",
DeprecationWarning,
)
allowed_methods = method_whitelist
if allowed_methods is _Default:
allowed_methods = self.DEFAULT_ALLOWED_METHODS
if remove_headers_on_redirect is _Default:
remove_headers_on_redirect = self.DEFAULT_REMOVE_HEADERS_ON_REDIRECT

self.total = total
self.connect = connect
self.read = read
Expand Down Expand Up @@ -287,6 +218,7 @@ def new(self, **kw):
redirect=self.redirect,
status=self.status,
other=self.other,
allowed_methods=self.allowed_methods,
status_forcelist=self.status_forcelist,
backoff_factor=self.backoff_factor,
raise_on_redirect=self.raise_on_redirect,
Expand All @@ -296,22 +228,6 @@ def new(self, **kw):
respect_retry_after_header=self.respect_retry_after_header,
)

# TODO: If already given in **kw we use what's given to us
# If not given we need to figure out what to pass. We decide
# based on whether our class has the 'method_whitelist' property
# and if so we pass the deprecated 'method_whitelist' otherwise
# we use 'allowed_methods'. Remove in v2.0
if "method_whitelist" not in kw and "allowed_methods" not in kw:
if "method_whitelist" in self.__dict__:
warnings.warn(
"Using 'method_whitelist' with Retry is deprecated and "
"will be removed in v2.0. Use 'allowed_methods' instead",
DeprecationWarning,
)
params["method_whitelist"] = self.allowed_methods
else:
params["allowed_methods"] = self.allowed_methods

params.update(kw)
return type(self)(**params)

Expand Down Expand Up @@ -421,19 +337,7 @@ def _is_method_retryable(self, method):
"""Checks if a given HTTP method should be retried upon, depending if
it is included in the allowed_methods
"""
# TODO: For now favor if the Retry implementation sets its own method_whitelist
# property outside of our constructor to avoid breaking custom implementations.
if "method_whitelist" in self.__dict__:
warnings.warn(
"Using 'method_whitelist' with Retry is deprecated and "
"will be removed in v2.0. Use 'allowed_methods' instead",
DeprecationWarning,
)
allowed_methods = self.method_whitelist
else:
allowed_methods = self.allowed_methods

if allowed_methods and method.upper() not in allowed_methods:
if self.allowed_methods and method.upper() not in self.allowed_methods:
return False
return True

Expand Down Expand Up @@ -573,20 +477,6 @@ def __repr__(self):
f"read={self.read}, redirect={self.redirect}, status={self.status})"
)

def __getattr__(self, item):
if item == "method_whitelist":
# TODO: Remove this deprecated alias in v2.0
warnings.warn(
"Using 'method_whitelist' with Retry is deprecated and "
"will be removed in v2.0. Use 'allowed_methods' instead",
DeprecationWarning,
)
return self.allowed_methods
try:
return getattr(super(), item)
except AttributeError:
return getattr(Retry, item)


# For backwards compatibility (equivalent to pre-v1.9):
Retry.DEFAULT = Retry(3)
10 changes: 1 addition & 9 deletions test/test_retry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import warnings
from unittest import mock

import pytest
Expand All @@ -15,13 +14,6 @@
from urllib3.util.retry import RequestHistory, Retry


@pytest.fixture(scope="function", autouse=True)
def no_retry_deprecations():
with warnings.catch_warnings(record=True) as w:
yield
assert len([str(x.message) for x in w if "Retry" in str(x.message)]) == 0


class TestRetry:
def test_string(self):
""" Retry string representation looks the way we expect """
Expand Down Expand Up @@ -282,7 +274,7 @@ def test_history(self):
)
assert retry.history == history

def test_retry_method_not_in_whitelist(self):
def test_retry_method_not_allowed(self):
error = ReadTimeoutError(None, "/", "read timed out")
retry = Retry()
with pytest.raises(ReadTimeoutError):
Expand Down