-
Notifications
You must be signed in to change notification settings - Fork 30
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
tweaks to not use celery AsyncResult and EagerResult for task status #89
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming task_status
to status
is the only required change I think.
operations = [ | ||
migrations.AddField( | ||
model_name='importtask', | ||
name='task_status', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably just be status
, no need to repeat task here.
SUCCESS = 'SUCCESS' | ||
FAILURE = 'FAILURE' | ||
|
||
READY_STATES = [SUCCESS, FAILURE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe FINAL_STATES
here? Ready seems a bit ambiguous. Also why not use a single character here? Maybe change SUCCESS
to COMPLETE
so there isn't overlap with S
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized you might be using these to keep compatibility with what the Celery states are. If so then that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I got those from Celery even if it is not guaranteed that the will always be in sync.
Also for task_status I wanted to keep the status method for backward compatibility in case it was called by someone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fair enough and a good reason. Looks good then, merge at will.
STARTED = 'STARTED' | ||
RUNNING = 'RUNNING' | ||
SUCCESS = 'SUCCESS' | ||
FAILURE = 'FAILURE' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COMPLETED
and FAILED
No description provided.