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

Added cache control header to enable caching. #3

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

Conversation

AJMansfield
Copy link

Just a small performance improvement.

These don't change over time. Get the browser to cache them to reduce traffic.
@azaghal
Copy link
Owner

azaghal commented Jul 18, 2017

Hi, sorry for the late reply, just a bit preoccupied. Glad that someone found the package useful, I was a bit surprised to see the PR for it since the package is a bit stale :)

There are a couple of changes that would be beneficial, though:

  1. It might be better to use django.views.decorators.cache.cache_control decorator on the view instead of hard-coding the header. See Django Docs.

  2. The max-age should be configurable (maybe calling it PYDENTICON_CACHE_MAX_AGE?), and set to 0 for backwards compatibility.

  3. Documentation should be updated to reflect the new option. This should include at least configuration.rst, and quite probably usage.rst as well (to draw user's attention to it since it can affect performance).

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