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

update behavior of app.result_expires to match celery docs #401

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

Conversation

jjorissen52
Copy link

@jjorissen52 jjorissen52 commented Jul 21, 2023

Celery documentation indicates that app.result_expires (or CELERY_RESULT_EXPIRES) should respect values of 0 or None: "A value of None or 0 means results will never expire (depending on backend specifications)."

Celery documentation indicates that app.result_expires of CELERY_RESULT_EXPIRES should respect values of 0 or None: "A value of None or 0 means results will never expire (depending on backend specifications)."
@@ -194,6 +194,8 @@ def _forget(self, task_id):

def cleanup(self):
"""Delete expired metadata."""
if not self.expires:
Copy link
Member

Choose a reason for hiding this comment

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

i would love to see unit test for this

Copy link
Author

Choose a reason for hiding this comment

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

This function is not covered in current unit tests, I didn't consider it the responsibility of this PR to add missing coverage. However, I can add a unit test.

Copy link
Member

Choose a reason for hiding this comment

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

you can add a basic unit test for this added change or that function as you like

Copy link
Member

Choose a reason for hiding this comment

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

it would be helpful if you can add a basic unit test so we can merge with confidence

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

I would like to merge it soon. any time to add some test? some manual acknowledge would be also fine, to make sure there are no regressions and

@TheWeirdDev
Copy link

Any updates on this?

@auvipy
Copy link
Member

auvipy commented Jun 8, 2024

Any updates on this?

we need either automated test, or manual acknowledgment for the proposed change

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