-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 Job.get_status and Job.restore to consistently return JobStatus Enum #2039
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2039 +/- ##
==========================================
+ Coverage 93.61% 93.76% +0.14%
==========================================
Files 28 29 +1
Lines 3758 3863 +105
==========================================
+ Hits 3518 3622 +104
- Misses 240 241 +1 ☔ View full report in Codecov by Sentry. |
rq/job.py
Outdated
@@ -322,18 +322,18 @@ def get_position(self) -> Optional[int]: | |||
return q.get_job_position(self._id) | |||
return None | |||
|
|||
def get_status(self, refresh: bool = True) -> JobStatus: | |||
def get_status(self, refresh: bool = True) -> Optional[JobStatus]: |
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.
I think get_status()
should always returns something. If the job hash in Redis has no status, raise an exception.
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.
Thank you for the note. I updated it to raise an exception if there's no status in Redis. I added a status=JobStatus.QUEUED
to a few Job.create
calls in other tests so that they wouldn't fail as a result. I don't think this changed the purpose or accuracy of those tests.
Returns: | ||
status (JobStatus): The Job Status | ||
""" | ||
if refresh: | ||
status = self.connection.hget(self.key, 'status') | ||
self._status = as_text(status) if status else None | ||
if not status: | ||
raise InvalidJobOperation(f"Failed to retrieve status for job: {self.id}") |
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.
Mind updating the test to test this code path so it can pass Codecov's test?
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.
sure thing!
Thanks! |
I wrote up what I found in issue #2038, so I though I should create a PR with my suggested fixes.