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

Timeout argument for ._create_job(), .enqueue_at(), enqueue_in() #124

Merged
merged 2 commits into from Jul 16, 2016

Conversation

lechup
Copy link
Contributor

@lechup lechup commented May 6, 2016

Scheduler._create_job()

  • now accepts also timeout argument
  • added test for above
  • cleanup of scheduler methods that were setting timeout not by _create_job

Scheduler.enqueue_at() + Scheduler.enqueue_in():

  • added timeout argument that is popped from kwargs

* added test for above
* cleanup of scheduler methods that was setting timeout not by `_create_job`
@selwin
Copy link
Collaborator

selwin commented May 6, 2016

@lechup thanks for the PR. Do you mind adding timeout argument to enqueue_in and enqueue_at methods as well?

* added timeout argument that is popped from kwargs
@lechup lechup changed the title Scheduler._create_job() timeout argument Timeout argument (._create_job(), .enqueue_at(), enqueue_in()) May 6, 2016
@lechup lechup changed the title Timeout argument (._create_job(), .enqueue_at(), enqueue_in()) Timeout argument for ._create_job(), .enqueue_at(), enqueue_in() May 6, 2016
@lechup
Copy link
Contributor Author

lechup commented May 6, 2016

@selwin I've prepared You asked here, but I'm not sure regarding how we can change arguments of enqueue_at/in I think that all Scheduler methods should have same logic of passing args/kwargs but it would be backward incompatible change. I've tried with addition of third argument but ended up with

======================================================================
FAIL: test_args_kwargs_are_passed_correctly (test_scheduler.TestScheduler)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/lechup/github/rq-scheduler/tests/test_scheduler.py", line 278, in test_args_kwargs_are_passed_correctly
    self.assertEqual(job.args, (1, 1, 1))
AssertionError: Tuples differ: (1, 1) != (1, 1, 1)

I've ended up with poping timeout from kwargs...

What I proposed is not clean solution either, if someone is passing timeout kwarg it won't be passed anymore and will end in unexpected behaviour.

I need Your help/decision here...

@lechup
Copy link
Contributor Author

lechup commented May 6, 2016

Oh and the commit is here: 316fdd9

@selwin
Copy link
Collaborator

selwin commented May 6, 2016

I'm fine with popping timeout from kwargs because that's also what happens in RQ's queue.enqueue. Thanks for working on this!

@lechup
Copy link
Contributor Author

lechup commented May 7, 2016

Hm... But still I think all Scheduler public methods should provide same way of passing args and kwargs for jobs... Place for another PR? ;)

I think we should go the way queue.enqueue is doing with arguments... Right?

@selwin
Copy link
Collaborator

selwin commented Jul 16, 2016

I don't mind introducing a slight backward incompatibility. To me it's more important that schedule, enqueue_at and enqueue_in share the same behavior. We can also make a note of this in the release notes. Thanks for your consistent contribution to RQ related libraries :)

@selwin selwin merged commit 5408650 into rq:master Jul 16, 2016
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

2 participants