-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Test on urllib3 1.26.x #6757
Test on urllib3 1.26.x #6757
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for testing urllib3 :)
src/requests/compat.py
Outdated
from urllib3 import __version__ as urllib3_version | ||
|
||
# Detect which major version of urllib3 is being used. | ||
is_urllib3_2 = urllib3_version.split('.')[0] == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a string, not an integer. Also, should do a >= check instead of equals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the int
. For the equivalence, I don't know if >=
is what we want if this is specifically scoping the 2.x major version. In the same way I wouldn't want an is_py3
to include Python 4.x. We could do something like is_gt_urllib3_1
but that seems like it may be premature forwards-compatibility?
Right now our dependencies are scoped at urllib3<3
and if we add this check to any other behaviors, I'd rather they stay scoped to the major version. That will let tests fail if we major version again and we can make an informed decision at that point when adding support. Otherwise, we may unintentionally carry forward behaviors that are subtly wrong.
I can see similar risks with both sides, so not a hill I'm going to die on, but that was my initial thought process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the check from checking for urllib3 2.x to check for 1.x. That leaves us open to forward compatibility without the confusing behavior with is_urllib3_2
including newer major versions.
d51bf99
to
81f1516
Compare
81f1516
to
4e38364
Compare
First step to address #6734. We'll enable testing on urllib3 1.26.x which was lost when we moved the pin to support 2.x. These will fail for now until the underlying issue is addressed, but this is to ensure we're catching any future breakages.