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

Test on urllib3 1.26.x #6757

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Test on urllib3 1.26.x #6757

merged 3 commits into from
Jul 30, 2024

Conversation

nateprewitt
Copy link
Member

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.

Copy link
Member

@sethmlarson sethmlarson left a 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 :)

from urllib3 import __version__ as urllib3_version

# Detect which major version of urllib3 is being used.
is_urllib3_2 = urllib3_version.split('.')[0] == 2
Copy link
Member

@sethmlarson sethmlarson Jul 18, 2024

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.

Copy link
Member Author

@nateprewitt nateprewitt Jul 18, 2024

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.

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'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.

@nateprewitt nateprewitt force-pushed the urllib-1.x-tests branch 2 times, most recently from d51bf99 to 81f1516 Compare July 18, 2024 17:08
@nateprewitt nateprewitt marked this pull request as ready for review July 18, 2024 17:47
@nateprewitt nateprewitt merged commit 79b74ef into psf:main Jul 30, 2024
27 checks passed
@nateprewitt nateprewitt deleted the urllib-1.x-tests branch July 30, 2024 01:44
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.

3 participants