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

Replace _to_bytes usages #94

Closed
Kernald opened this issue May 31, 2021 · 3 comments · Fixed by #97
Closed

Replace _to_bytes usages #94

Kernald opened this issue May 31, 2021 · 3 comments · Fixed by #97

Comments

@Kernald
Copy link

Kernald commented May 31, 2021

As a fix to #92 , #93 added a block to the requests-cache version used (as 0.6.0 removed _to_bytes). I believe it was never meant to be used by external users, and should be quite straightforward to replace.

I'm opening this issue so this doesn't slip by unattended, as it's causing issues in packaging (on distributions that only have one version of a specific package, e.g. requests-cache).

While I'd be happy to contribute a fix, I'm not familiar with Python so I have a question. According to the readme, this repo officially supports Python 2.7 and onward, but I was under the assumption that Python 2 had been EOL'd over a year ago. Is Python 2 compatibility still something desirable? _to_bytes was removed from requests-cache in an effort to clean-up the code at the expense of Python 2 compatibility, so I guess upgrading the requests-cache version comes at the cost of said compatibility already.

@dbr
Copy link
Owner

dbr commented May 31, 2021

I was under the assumption that Python 2 had been EOL'd over a year ago. Is Python 2 compatibility still something desirable?

Not any more - I haven't developed tvdb_api significantly since before 2.7 was EOL, which is the only reason the support is still there!

Dropping support for Python 2.7 is planned (#86) and was planning on doing that a tvdb_api v4

I suspect there is a better way to be using requests-cache which doesn't rely on internal methods, as the problematic create_key method (which uses _to_bytes and other _private methods) is only there to workaround the fact it was including the always-changing auth token which made the cache ineffective.

It seems like ignored_parameters is the better approach (I don't think this option existed at the time, the workaround was added in 2017)

So I think the best approach is:

  1. Leave tvdb 3 pinned to the old requests-cache release. This leaves tvdb_api supporting Python 2.7 for what its worth
  2. Slightly accelerate tvdb_api v4, which will drop 2.7 support and also uses the ignored_parameters option in requests-cache instead of the create_key workaround which caused all this issue in first place.

By "slightly accelerated" I mainly mean the changes in the 3.2 milestone could easily get shuffled into the 4.0 release

@fabaff
Copy link

fabaff commented Jul 8, 2021

The missing support for requests-cache>0.6.0 breaks the NixOS package. It would be nice if later requests-cache could be supported. Thanks.

@JWCook
Copy link

JWCook commented Oct 1, 2022

@dbr
Hi there, I maintain requests-cache, and came across this issue while looking into dbr/tvnamer#210.

Later versions of requests-cache add some other features that would be relevant to you. In 0.8+, you can pass a list of specific headers to match instead of just True/False, for example:

session = CachedSession(match_headers=['Accept-Language'])

And if the builtin options aren't enough, it also lets you pass a callback instead of patching out create_key, for example:

session = CachedSession(key_fn=my_custom_key_function)

More docs on request matching can be found here: https://requests-cache.readthedocs.io/en/stable/user_guide/matching.html

Also, the upcoming version of requests-cache will ignore common authentication headers by default, and also respect Vary headers. So if, for example, the server provides Vary: Accept-Language, that will be handled automatically.

That doesn't help you right now, but if/when you get around to dropping support for older python versions, you are more than welcome to ping me for questions related to requests-cache, or create an issue here.

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 a pull request may close this issue.

4 participants