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.

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

Rename Retry options and defaults #2000

Merged
merged 6 commits into from Sep 28, 2020

Conversation

sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Sep 25, 2020

Closes #1916

Still need to add some test cases on how setting both options interacts.

@sethmlarson sethmlarson force-pushed the rename-retry-options branch 2 times, most recently from a642692 to 43db004 Compare September 25, 2020 18:51
# and if so we pass the deprecated 'method_whitelist' otherwise
# we use 'method_allowlist'. Remove in v2.0
if "method_whitelist" not in kw and "method_allowlist" not in kw:
if "method_whitelist" in self.__dict__:
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm doing this instead of hasattr(self, "method_whitelist") because that triggers a getattr() call. Thanks Hynek for the blog post. If there's a better way, let me know :)

@@ -0,0 +1,428 @@
import mock
Copy link
Member Author

Choose a reason for hiding this comment

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

This is literally a copy-paste of test_retry.py with extra asserts about deprecated options. Done so that we can move along with test_retry.py without having to think about deprecations much but also ensure that everything that used to work still does.

We'll remove this from master once we cut a v1.26 branch

@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #2000 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2000   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         2187      2233   +46     
=========================================
+ Hits          2187      2233   +46     
Impacted Files Coverage Δ
src/urllib3/util/retry.py 100.00% <100.00%> (ø)
src/urllib3/util/ssl_.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a5c028...0993ad4. Read the comment docs.

@sethmlarson sethmlarson force-pushed the rename-retry-options branch 2 times, most recently from d6676a9 to ef831d4 Compare September 27, 2020 16:58
src/urllib3/util/retry.py Outdated Show resolved Hide resolved
src/urllib3/util/retry.py Show resolved Hide resolved
@shazow
Copy link
Member

shazow commented Sep 27, 2020

This is great, will feel good to purge this in v2.0 :)

@sethmlarson
Copy link
Member Author

Thanks for review comments @sigmavirus24 and @shazow, rerequesting reviews from both of yall with the changes :)

shazow
shazow previously approved these changes Sep 28, 2020
Copy link
Member

@shazow shazow left a comment

Choose a reason for hiding this comment

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

Looks good, only minor/optional suggestions. :)

_Default = object()


class RetryMeta(type):
Copy link
Member

Choose a reason for hiding this comment

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

Optional nit: Maybe RetryDeprecations or RetryWarnings would be more descriptive.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to pass on this one since it's "Retry metaclass", maybe I should make the class private though.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I just haven't written Python in too long, it has become weird to me to add type hinting into the name of things (rather than describe what they do).

src/urllib3/util/retry.py Outdated Show resolved Hide resolved
test/test_retry_deprecated.py Outdated Show resolved Hide resolved
test/test_retry_deprecated.py Show resolved Hide resolved
Co-authored-by: Andrey Petrov <shazow@gmail.com>
sethmlarson and others added 3 commits September 28, 2020 09:29
Co-authored-by: Andrey Petrov <shazow@gmail.com>
Co-authored-by: Andrey Petrov <shazow@gmail.com>
Copy link
Contributor

@sigmavirus24 sigmavirus24 left a comment

Choose a reason for hiding this comment

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

I started down a path of "There's got to be a better way than using a metaclass" for that and ended up in a dark dark place of Python I might have to turn into a library to make better.

@shazow
Copy link
Member

shazow commented Sep 28, 2020

@sigmavirus24 git init deprecatelib3

@sigmavirus24
Copy link
Contributor

Tag line: break shit more often 🤘

@shazow
Copy link
Member

shazow commented Sep 28, 2020

DEPRECATE EVERYTHING / FEAR NOTHING!

@sethmlarson sethmlarson merged commit 382ab32 into urllib3:master Sep 28, 2020
@sethmlarson sethmlarson deleted the rename-retry-options branch September 28, 2020 17:45
@sethmlarson
Copy link
Member Author

Thanks for the reviews @shazow and @sigmavirus24!

This was referenced Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace method_whitelist and DEFAULT_REDIRECT_HEADERS_BLACKLIST parameter names for the Retry method
5 participants