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

Drop repoze.lru on Python 3 #3140

Merged
merged 1 commit into from
Oct 22, 2017
Merged

Drop repoze.lru on Python 3 #3140

merged 1 commit into from
Oct 22, 2017

Conversation

bochecha
Copy link
Contributor

@bochecha bochecha commented Aug 3, 2017

Starting with Python 3.2, the functools module grew a lru_cache function which can replace our usage of repoze.lru.

@mmerickel
Copy link
Member

Thanks! One of the maintainers will need to do some legwork to determine if this library actually covers the required features or not.

Environment markers are the correct way to handle conditional dependencies right now. For example: https://github.com/Pylons/pyramid_debugtoolbar/blob/7c32a1fc909e182f2f26ed5cef680784ffde6263/setup.py#L77

setup.py Outdated
@@ -35,6 +36,9 @@ def readfile(name):
'hupper',
]

if sys.version_info < (3, 2):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an install_requires tag instead, not a dynamic change, this way it ends up correctly in the wheels and things work as expected.

pyramid/url.py Outdated
@@ -2,7 +2,11 @@

import os

from repoze.lru import lru_cache
try:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to compat.py and import from compat.py instead of repeating this try/importerror everywhere.

Starting with Python 3.2, the functools module grew a lru_cache function
which can replace our usage of repoze.lru.
@bochecha
Copy link
Contributor Author

bochecha commented Aug 4, 2017

One of the maintainers will need to do some legwork to determine if this library actually covers the required features or not.

They both implement the same thing (an LRU cache for function results), but indeed have subtle differences in behaviour, which would be better checked by a maintainer.

One such difference that I can see from a quick glance at the repoze.lru code is that it seems to allow expiring things after a certain time (with its timeout kwarg) which functools.lru_cache doesn't support.

functools.lru_cache also supports optionally caching results by type, but not repoze.lru.lru_cache.

However the subset of the API that is used in the Pyramid code base seems to work similarly in both cases.

They also handle differently a maxsize argument set to 0 or None, but that isn't used in Pyramid either.

mmerickel added a commit that referenced this pull request Oct 22, 2017
@mmerickel mmerickel merged commit e80491c into Pylons:master Oct 22, 2017
@mmerickel
Copy link
Member

Thanks @bochecha, I reviewed the two codebases and could not see a compelling reason not to move forward with functools.lru_cache.

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