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

Callback for Job success #969

Closed
wants to merge 7 commits into from

Conversation

iamrajhans
Copy link

Call function after job is completed successfully.

@theodesp
Copy link
Contributor

theodesp commented Jun 12, 2018

@iamrajhans First you need to add test cases and then you need to fix the CI conflicts before we review it

@iamrajhans
Copy link
Author

@theodesp okay will add test cases

@dralley
Copy link
Contributor

dralley commented Dec 18, 2019

@iamrajhans ?

@iamrajhans
Copy link
Author

@theodesp will add the test cases and fix the pipeline

@theodesp
Copy link
Contributor

Sure go ahead!

@codecov
Copy link

codecov bot commented Dec 21, 2019

Codecov Report

Merging #969 into master will decrease coverage by 0.03%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #969      +/-   ##
==========================================
- Coverage   93.62%   93.59%   -0.04%     
==========================================
  Files          22       22              
  Lines        2260     2343      +83     
==========================================
+ Hits         2116     2193      +77     
- Misses        144      150       +6     
Impacted Files Coverage Δ
rq/worker.py 90.19% <66.66%> (-0.61%) ⬇️
rq/cli/cli.py 91.71% <100.00%> (+0.57%) ⬆️
rq/decorators.py 100.00% <0.00%> (ø)
rq/registry.py 98.78% <0.00%> (+<0.01%) ⬆️
rq/utils.py 92.68% <0.00%> (+0.05%) ⬆️
rq/cli/helpers.py 86.36% <0.00%> (+0.08%) ⬆️
rq/scheduler.py 96.87% <0.00%> (+0.10%) ⬆️
rq/job.py 97.46% <0.00%> (+0.17%) ⬆️
rq/queue.py 93.54% <0.00%> (+0.25%) ⬆️
... and 1 more

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 eb92d68...9fbdecb. Read the comment docs.

@iamrajhans
Copy link
Author

@theodesp please review

@iamrajhans
Copy link
Author

@theodesp could you please review

@williamfzc
Copy link

looking forward to this great feature

@iamrajhans
Copy link
Author

@theodesp please review the changes

@iamrajhans
Copy link
Author

@theodesp, please review the changes.

@iamrajhans
Copy link
Author

@dralley @theodesp could you please review

@trodery
Copy link

trodery commented Apr 18, 2020

codecov is reporting 72.72% coverage of the diff with a target of 93.11%. Perhaps if you were able to reach the coverage target it would make the PR easier for the maintainers?

@@ -950,6 +960,18 @@ def handle_exception(self, job, *exc_info):
if not fallthrough:
break

def success_job_callback(self,job,*exc_info):
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the job is successfully executed, there's no need to pass exc_info as arguments.

@@ -960,6 +982,10 @@ def _get_safe_exception_string(exc_strings):
exc_strings = [exc.decode("latin-1") for exc in exc_strings]
return ''.join(exc_strings)

def push_job_success_handler(self,handle_func):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this method, you can directly call self._job_success_handlers.append()

@@ -215,6 +217,12 @@ def __init__(self, queues, name=None, default_result_ttl=DEFAULT_RESULT_TTL,
self.push_exc_handler(handler)
elif exception_handlers is not None:
self.push_exc_handler(exception_handlers)

if isinstance(success_handlers,list):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should check whether this is an iterable and not just a list.

@@ -232,6 +233,10 @@ def worker(cli_config, burst, logging_level, name, results_ttl,
for h in exception_handler:
exception_handlers.append(import_attribute(h))

success_handlers = []
for s_h in success_handler:
Copy link
Collaborator

Choose a reason for hiding this comment

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

for handler in success_handler:

@selwin
Copy link
Collaborator

selwin commented May 3, 2020

Can you run your PR through flake 8 and address all the warnings there?

@selwin
Copy link
Collaborator

selwin commented May 3, 2020

Also, please add more comprehensive tests so we can hit the diff target.

@selwin
Copy link
Collaborator

selwin commented Nov 14, 2020

Closing this PR due to inactivity.

@selwin selwin closed this Nov 14, 2020
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.

6 participants