tkem / cachetools

Extensible memoizing collections and decorators
MIT License
2.33k stars 163 forks source link

maxsize=None doesn't work with python3 #141

Closed ndamclean closed 5 years ago

ndamclean commented 5 years ago

comparisons with None and int values raise errors in python 3

tkem commented 5 years ago

Nice catch! Though I could swear, I already had a unit test for that... Will look into your PR ASAP!

tkem commented 5 years ago

After looking into your PR, I realized I misinterpreted this. Actually, maxsize=None is only supported for the cachetools.func decorators: https://cachetools.readthedocs.io/en/latest/#module-cachetools.func What makes you think passing maxsize=None to a Cache constructor should be valid? Because it's not. So that this raises an error in Python 3 is the desired behavior. Maybe this should have been checked explicitly for Python 2, but it's a little late for that now.

ndamclean commented 5 years ago

I see. The docs don't mention whether maxsize can be None for Cache object instances, so I had incorreclty assumed that whis would be valid as it is for the decorator API.

If that's the desired behaviour, I think an error should be raised in the constructor if maxsize is set to None instead of raising an error after you attempt to store a value in the cache.

ndamclean commented 5 years ago

IMO the ability to set maxsize to None to unlimited would be a useful feature for some caching strategies (e.g. TTLCache)

tkem commented 5 years ago

@ndamclean: Regarding your first comment: They don't mention that maxsize can be set to None, either ;-) They also state in the introduction, that

For the purpose of this module, a cache is a mutable mapping of a fixed maximum size.

But I agree, the docs could state this more clearly. Given that this seems to work with Python 2.7, which I didn't realize, it may surprise people porting their code to Python 3. So I'll consider that, thanks!

Regarding your second comment, I do disagree somewhat. For three out of four cache implementations (LRU, LFU, RR) this doesn't make sense really, and you'd be better off using a plain dict (which you can also use with the caching decorators). For TTL, you'd really want an "expiring dict", which doesn't have the additional overhead of checking maxsize, etc. Maybe someone has already implemented something like that, otherwise feel free to adapt the code from TTLCache accordingly.

That being said, it actually is already possible to achieve the desired behavior without explicitly supporting it or me encouraging this: TTLCache(maxisze=float('inf')) should do the trick.

ndamclean commented 5 years ago

@tkem Actually, I just tested this in python 2.7.

from cachetools import TTLCache
tc = TTLCache(maxsize=None, ttl=10)
tc[0] = 0

This raises ValueError: value too large, so it seems as if in python 2, maxsize=None is the same as maxsize=0, which seems like sensible behaviour.

I guess you don't think you need to worry about changing anything.

Thanks for the TTLCache(maxisze=float('inf')) suggestion.

tkem commented 5 years ago

Well, kudos to @sublee, who brought this up in #73.

sublee commented 5 years ago

@tkem Thank you for your respect :)

gandhis1 commented 2 years ago

Why exactly does maxsize=None not make sense for something like TTLCache? This is the convention in functools, it's pretty well understood. I'm fine using float("inf") but this is counter to what I think most would expect.