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

Added queue_name attribute to JobResult #198

Merged
merged 2 commits into from
Apr 21, 2021

Conversation

ccharlesgb
Copy link
Contributor

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.

@codecov
Copy link

codecov bot commented Jul 29, 2020

Codecov Report

Merging #198 (3ce4cb0) into master (b8dd9ee) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 3ce4cb0 differs from pull request most recent head 66ad5da. Consider uploading reports for the commit 66ad5da to get more accurate results

@@           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           
Impacted Files Coverage Δ
arq/worker.py 99.49% <ø> (ø)
arq/jobs.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8dd9ee...66ad5da. Read the comment docs.

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'],
Copy link
Contributor Author

@ccharlesgb ccharlesgb Jul 29, 2020

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') ?

Copy link
Member

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.

@ccharlesgb
Copy link
Contributor Author

@samuelcolvin Sorry to nudge. But would be great to get your thoughts on this

@samuelcolvin samuelcolvin merged commit d594c9c into python-arq:master Apr 21, 2021
@samuelcolvin
Copy link
Member

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.

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.

2 participants