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

tweaks to not use celery AsyncResult and EagerResult for task status #89

Merged
merged 2 commits into from
Dec 19, 2016

Conversation

norkans7
Copy link
Contributor

No description provided.

Copy link
Collaborator

@nicpottier nicpottier left a 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',
Copy link
Collaborator

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]
Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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'
Copy link
Collaborator

Choose a reason for hiding this comment

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

COMPLETED and FAILED

@coveralls
Copy link

coveralls commented Dec 19, 2016

Coverage Status

Coverage increased (+0.5%) to 79.719% when pulling b9069fa on task-status-field into 3bcf84e on master.

@norkans7 norkans7 merged commit 495980a into master Dec 19, 2016
@norkans7 norkans7 deleted the task-status-field branch December 19, 2016 14:21
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.

None yet

3 participants