-
-
Notifications
You must be signed in to change notification settings - Fork 176
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
Added queue_name attribute to JobResult #198
Added queue_name attribute to JobResult #198
Conversation
Codecov Report
@@ Coverage Diff @@
## master #198 +/- ##
=======================================
Coverage 99.73% 99.74%
=======================================
Files 9 9
Lines 769 770 +1
Branches 107 107
=======================================
+ Hits 767 768 +1
Misses 1 1
Partials 1 1
Continue to review full report at Codecov.
|
arq/jobs.py
Outdated
@@ -247,6 +250,7 @@ def deserialize_result(r: bytes, *, deserializer: Optional[Deserializer] = None) | |||
result=d['r'], | |||
start_time=ms_to_datetime(d['st']), | |||
finish_time=ms_to_datetime(d['ft']), | |||
queue_name=d['q'], |
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 was slightly concerned that this change will break compatibility if the worker is running an older version of arq, then it would cause this to raise a KeyError. Am I right in this thinking and should I change this to d.get('q')
?
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.
yes I think you're right.
Since it's been so long, I'll fix this myself now.
@samuelcolvin Sorry to nudge. But would be great to get your thoughts on this |
So sorry for the massive delay on this, thank you so much for your contribution. I've made the change you suggested and merged this. I'm working on a new release now. |
This pull request addresses issue #161
I didn't feel like I needed to add any new tests as modifying the existing tests checks that the attribute exists in the result.
Also technically I could add the queue_name attribute in the JobDef as well but I don't really see a use for that yet but happy to add it there as well.