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

Add default timeout to cache memoize #46

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

Conversation

KarimTayie
Copy link

No description provided.

@peterbe
Copy link
Owner

peterbe commented Jan 4, 2021

Can't you just leave it instead? I.e. if you do:

>>> from django.core.cache import cache
>>> cache.set('foo', 'bar')

it will apply whatever's in django.conf.settings.CACHES["default"].

@peterbe
Copy link
Owner

peterbe commented Jan 4, 2021

Oh I see. I think you can just use:

>>> from django.core.cache import cache
>>> cache.set('foo', 'bar', timeout=object())

is the exact same as not specifying it.

Copy link
Owner

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

Actually. It makes sense.

Personally, I never use the default because if you're caching properly you probably need to have better understanding and control over the value because it depends on business logic and the application.

@peterbe
Copy link
Owner

peterbe commented Jan 4, 2021

I can't merge this because the CI tests aren't running. Any idea why?

@KarimTayie
Copy link
Author

Hello,
sorry for taking so long to reply.

I can't merge this because the CI tests aren't running. Any idea why?

I can't find the CI tests, can you please show me the logs?

@peterbe peterbe closed this Jan 20, 2021
@peterbe
Copy link
Owner

peterbe commented Jan 20, 2021

Let's see if this restarts the CI tests.

@peterbe peterbe reopened this Jan 20, 2021
@peterbe
Copy link
Owner

peterbe commented Jan 20, 2021

Can you merge in the latest upstream master into your branch and push. Perhaps that'll pick up the new GitHub Actions checks.

@KarimTayie
Copy link
Author

From github.com:peterbe/django-cache-memoize
 * branch            master     -> FETCH_HEAD
Already up to date.

already pulled the latest upstream master into this pr.
Screenshot from 2021-01-21 23-31-46
from this screenshot, all looks good.

@peterbe
Copy link
Owner

peterbe commented Jan 22, 2021

Let's try if close and re-opening triggers the CI tests.

@peterbe peterbe closed this Jan 22, 2021
@peterbe peterbe reopened this Jan 22, 2021
@peterbe
Copy link
Owner

peterbe commented Jan 22, 2021

What the heck?! I don't know why the tests aren't running. I'll have to take a look some day when I have time.

@YPCrumble
Copy link
Contributor

@KarimTayie @peterbe my update in #48 should make the tests run via 8470d5b

@peterbe
Copy link
Owner

peterbe commented Mar 29, 2021

@KarimTayie can you merge in master into your branch and push that that should hopefully fix your tests.

@EmberCraze
Copy link
Contributor

Hey, can we get this done? I can create a similar PR and get the CI to run if needed. Would that be okay?

@peterbe
Copy link
Owner

peterbe commented Dec 13, 2024

Hey, can we get this done? I can create a similar PR and get the CI to run if needed. Would that be okay?

Yeah, start a fresh one.

@EmberCraze
Copy link
Contributor

Hey, can we get this done? I can create a similar PR and get the CI to run if needed. Would that be okay?

Yeah, start a fresh one.

Here it is #68

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.

4 participants