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

fix: compatibility with urllib3 2.0 #294

Closed
wants to merge 1 commit into from

Conversation

frostming
Copy link
Contributor

@frostming frostming commented May 5, 2023

Signed-off-by: Frost Ming [email protected]

Fix #292
Fix #293

@adiroiban
Copy link

Thanks @frostming for the change.

I see that besides the strict keyword removal this PR also does import reordering and some reformating and modernization changes.


Also for older urllib versions, this will just ignore the strict argument.

I think that we should wait for @ionrock to review this.

I was expecting to remove strict only for newer urllib versions, so that cachecontrol will continue to work with both old and new urllib versions.

@ionrock
Copy link
Contributor

ionrock commented May 8, 2023

I agree with @adiroiban that we should avoid formatting changes. The caveat is unless black reformats something, in which case two commits might be helpful to communicate that is what is happening. I'd also skip the setup to setup_method as it isn't necessary.

I also think it is worthwhile to handle the earlier urllib versions. When I started looking at it, I had considered a function in compat.py that checked the version and would remove the strict field there based on the version.

Thanks for diving into this!

@miigotu
Copy link

miigotu commented May 9, 2023

Why not just set the minimum version of urllib supported by the next version, so that earlier urllib versions will just use the older cachecontrol? Trying to be backwards compatible with an obsolete package version seems unnecessary when a working version of cachecontrol already exists for those older versions. Sort of what versioning and pinning is for imo.

@frostming
Copy link
Contributor Author

frostming commented May 10, 2023

Why not just set the minimum version of urllib supported by the next version

I agree with this strategy. Requesting a library to support all possible Python versions and urllib3 versions is a pain for maintainers. Also we seem to have to drop Python 3.6 in the CI matrix. And eventually support for Python <3.7 should be dropped and compat.py isn't needed any more.

@adiroiban
Copy link

Good feedback.

Only supporting the latest urllib3 is a valid point, but then, this PR should update setup.py and declare this explicit dependency.

install_requires=["requests", "msgpack>=0.5.2"],

Thanks

@jacobg
Copy link

jacobg commented May 22, 2023

Hi, any status update on this one?

@frostming
Copy link
Contributor Author

Hi, I have to move on to create a fork at https://github.com/frostming/cacheyou, with this issue addressed.

The long term plan is if this project can continue I will merge the changes back.

@dairiki
Copy link
Contributor

dairiki commented May 23, 2023

I also think it is worthwhile to handle the earlier urllib versions. When I started looking at it, I had considered a function in compat.py that checked the version and would remove the strict field there based on the version.

Note that (when running under Python 3) urllib3==1.* simply discards and ignores any strict parameter passed to the HTTPConnection constructor. It has been this way since urllib3==1.8 (circa 2014, see urllib3/urllib3#343). (It is also noted in the urllib3 docs that that strict has no effect outside of Python 2.)

Since cachecontrol declares Requires-Python: >=3.6, it seems that it is pretty safe to simply remove the strict field.

[ETA: Also note that the strict parameter hasn't been supported by http.client.HTTPResponse since before Python 3.2.]

[ETA: IOW, the strict parameter, even where accepted, doesn't actually do anything under any extant version of Python 3. AFAICT, it can be safely ignored/dropped unless the intent is to support Python 2.]

@miigotu
Copy link

miigotu commented May 25, 2023

Hi, I have to move on to create a fork at https://github.com/frostming/cacheyou, with this issue addressed.

The long term plan is if this project can continue I will merge the changes back.

Thanks for that, I was close to doing the same thing but I'm glad someone stepped up for it. Sickchill is using your fork for the foreseeable future since cachecontrol hasn't been updated in years. I hope you can keep it relevant and set up a nice GitHub actions workflow to keep it always maintained and supporting the latest versions of everything. Godspeed.

@miigotu
Copy link

miigotu commented May 25, 2023

I also think it is worthwhile to handle the earlier urllib versions. When I started looking at it, I had considered a function in compat.py that checked the version and would remove the strict field there based on the version.

Note that (when running under Python 3) urllib3==1.* simply discards and ignores any strict parameter passed to the HTTPConnection constructor. It has been this way since urllib3==1.8 (circa 2014, see urllib3/urllib3#343). (It is also noted in the urllib3 docs that that strict has no effect outside of Python 2.)

Since cachecontrol declares Requires-Python: >=3.6, it seems that it is pretty safe to simply remove the strict field.

[ETA: Also note that the strict parameter hasn't been supported by http.client.HTTPResponse since before Python 3.2.]

[ETA: IOW, the strict parameter, even where accepted, doesn't actually do anything under any extant version of Python 3. AFAICT, it can be safely ignored/dropped unless the intent is to support Python 2.]

Just supporting the latest on python 3.8+ is ideal, backwards compatibility was a huge importance a few years ago before most projects finally got out of the python 2.7 trap. With GitHub actions and all the great tools now almost everyone has been able to keep their dependency trees updated thanks to dependabot. Older versions will just get the older cachecontrol with the pip dependency resolver so nobody misses anything to drop backwards compatible.

@frostming
Copy link
Contributor Author

Closed in favor of #301, which provides a cleaner fix without format changes, thanks @dairiki .

@frostming frostming closed this May 25, 2023
@frostming frostming deleted the fix/urllib3-2 branch May 25, 2023 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants