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.

Sign up for GitHub

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

cache return #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cache return #22

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions __init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import json, os
import six
from io import open
from django.core.cache import cache

try:
langs
Expand All @@ -19,17 +20,22 @@
else:
langs_by_code[lang['three']] = lang


def get_name(lang):
lang_info = langs_by_code.get(lang)
if lang_info:
return lang_info['names'][0]
else:
return None

def get_all_langs_for_select():
langs_for_select = []
for key, val in six.iteritems(langs_by_code):
label = key + " (" + val['names'][0] + ")"
langs_for_select.append((key, label))

def get_all_langs_for_select():
langs_for_select = cache.get('__all_langs_for_select')
if not langs_for_select:
langs_for_select = []
for key, val in six.iteritems(langs_by_code):
label = key + " (" + val['names'][0] + ")"
langs_for_select.append((key, label))
cache.set('__all_langs_for_select', langs_for_select, 12 * 60 * 60)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the cache get invalidated when new langs are added? If not, does that matter? Could it cause frustration if someone added a new language, but it doesn't appear in some places until up to 12 hours later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New langs can be added only via code. but i see what you mean, even after deploy it would not show for a while.

Does this look like the right place to just delete the cache?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, that code... unless there is some import magic going on there, it looks like the except clause will always run on module import. You didn't write that code, but it's generally a bad practice to do things like accessing external resources (reading files or anything that could raise an error due to environment (mis)configuration) at module import time. A better practice is to do that sort of thing the first time the the resource is actually needed.

If you add the cache clearing logic there it would require the cache backend service (redis?) to be available when that module is imported the first time, which probably means any time a management command is run or whatever. For example, it would likely make it impossible to run ./manage.py makemigrations unless redis is running, which seems like a completely unnecessary dependency chain.

It looks like that module and any code that touches the langs variable should be refactored to defer referencing langs until after the initial import, and should access it by calling a function (named something like get_langs()), which can either clear and initialize the cache (the first time it is called) or return a cached copy, or whetever. Without digging into the code I'm not sure how much work that will be.

return langs_for_select