Added args and kwargs implementation #24
Conversation
Kwargs are set by parsing a json formatted string Args are passed in as a string "a, b, etc." Admin list display shows the function including args & kwargs Fixed issue with tests where naive datetime was being sent when tz-support is active.
tom-price
force-pushed
the
kwargs+args
branch
6 times, most recently
from
05a8a86 to
265cd3b
Compare
Model (not DB) validation prevents more than one of the potential fields from being set. Additional Javascript hides all but one value field in admin interface. Tests need to be written, but according to Factory Boy documentation "All factories for a Django Model should use the DjangoModelFactory base class", so I'll likely address that first.
|
Significant updates were made to this, but it's still not quite ready to move the args and kwargs into their own models. Tests are pending me learning how to properly use Factory Boy. The admin interface looks like this now: Lastly, really hope the other reviewers weren't spammed w. notifications. Was forcing some updates a branch that I didn't realize was attached to this PR. |
tom-price
changed the title
Models had some duplicate code removed and layout changed for better consistency. The tests were re-structured to better leverage Factory Boy and inherit from DjangoModelFactory, which was introduced in v2.0. They were also re-organized for clarity when inheriting. FakeStrictRedis runs in place of the redis client as per http://python-rq.org/docs/testing/. Additional tests were written to increase test coverage of models file.
Merged re-written tests and updates into branch.
Key improvements include testing the pass-through of parameters to the created Job object.
Several duplicated modelAdmin entries moved into mixin. Added search of name and callable fields to admin. Instead of enabled being a list_editable field there's now a form action to enable or disable all in a selection. This is useful when paired with the list_filter and search fields allowing for bulk enabling / disabling of jobs.
Several duplicated modelAdmin entries moved into mixin. Added search of name and callable fields to admin. Instead of enabled being a list_editable field there's now a form action to enable or disable all in a selection. This is useful when paired with the list_filter and search fields allowing for bulk enabling / disabling of jobs.
|
@oudeismetis and @bashhack, I'm hoping to get your thoughts on this. The task was quite a lot more complex than I initially thought, but now I think it's written in a pretty extensible way. There are also several revisions to the codebase not directly tied to the args + kwargs implementation, but without making them things would have been messier (the changes to tests are an example). I'm on vacation until August 22nd, so no rush on this. I've been using these changes in my own project for the past few weeks without issue in my project. I'm concerned that the added filter by Callable in list view, while it makes sense in my project and where args + kwargs are involved, may just end up being a very long list for others causing performance issues. |
schedule function for ScheduledJobs return false if date in past schedule function for RepeatableJobs returns false if date + (interval * repeat) ie. time of last run is in past else saves with date in future and new remaining count.
|
Thanks for your work on this Tom, I hope it gets merged some day! The requirement to create a new callable for every job is a big problem in many contexts, but especially in data-oriented web apps! |
|
I've got some ideas for improvements and have substantially more knowledge about how to optimize code that runs in a Django web-app than I did when I wrote this last summer (current performance is fine for my use... but I'm not everyone). So, time allowing I'm going to make some tweaks, review any potential merge conflicts, and re-light the fire to get this merged in. |
|
New implementation available in PR #35 |
Kwargs are set by parsing a json formatted string
Args are passed in as a string "a, b, etc."
Admin list display shows the function including args & kwargs
Fixed issue with tests where naïve datetime was being sent when tz-support is active.
I built this out of necessity and also issue #1. This implementation covers my needs but maybe should be expanded before incorporating and I'd love some feedback on the implementation of this.
The args field is more limited than the kwargs field as it'll only accept strings. Was considering creating another model to hold args and kwargs, which would make it more easy to support args of different types (thinking Datetime specifically). If it's worth doing it's probably worth doing now as otherwise future migrations will be a pain in the ass.
The tests also would benefit from some feedback.