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

cache return #22

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

cache return #22

wants to merge 1 commit into from

Conversation

mkangia
Copy link
Contributor

@mkangia mkangia commented Aug 23, 2018

No description provided.

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.

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.

2 participants