Feature/custom schedule validators #361
Conversation
* Always call validators with a suitable list of items * Add prefetch_slot helper for calling slot validation routines * Update test cases
docs/schedule.rst
Outdated
| return a list of invalid items or an empty list if the validator finds | ||
| no error. | ||
|
|
||
| Use ``register_schedule_item_validator`` and ``register_slot_vaildator`` |
docs/schedule.rst
Outdated
| Wafer runs validation on the slots and the schedule items. This behaviour | ||
| can be extended by providing custom validators. | ||
|
|
||
| Each validator is called with a list of all the items and is expected to |
There was a problem hiding this comment.
Should we explain that item validators get a list of items and slot validators get a list of slots?
|
|
||
| # Helpers for people extending the tests | ||
| def register_slot_validator(function, err_type, msg): | ||
| global SLOT_VALIDATORS |
There was a problem hiding this comment.
Given that we're calling .append(...) we don't actually need the globals here but I guess it's not a bad idea to make it clear that these are globals. Happy either way, just checking intent.
There was a problem hiding this comment.
I personally prefer the explicit globals declaration for clarity.
wafer/schedule/admin.py
Outdated
| for validator, err_type, _msg in SCHEDULE_ITEM_VALIDATORS: | ||
| failed_items = validator(all_items) | ||
| if failed_items: | ||
| errors[err_type] = failed_items |
There was a problem hiding this comment.
I'm a little worried this can override errors if err_type is not unique (and the registration interface doesn't prevent that). Maybe we could do:
errors = defaultdict(list)
...
errors[err_type].extend(failed_items)?
There was a problem hiding this comment.
Good point - I'll make that change.
wafer/schedule/admin.py
Outdated
| for validator, err_type, _msg in SLOT_VALIDATORS: | ||
| failed_slots = validator(all_slots) | ||
| if failed_slots: | ||
| errors[err_type] = failed_slots |
There was a problem hiding this comment.
Same question about uniqueness of err_type here.
| @@ -8,6 +8,7 @@ | |||
| {% if errors %} | |||
| <div name="errors"> | |||
| <h2>{% trans "Errors in the slots" %}</h2> | |||
| {% block displayerrors %} | |||
| {% if errors.overlaps %} | |||
| <h3>{% trans "OVerlapping slots" %}</h3> | |||
docs/schedule.rst
Outdated
| Use ``register_schedule_item_validator`` and ``register_slot_vaildator`` | ||
| to add the validators to the list. | ||
|
|
||
| To display the errors in the admin form, you will also need to extend the |
There was a problem hiding this comment.
Not necessarily for this PR, but it would be cool if the default displayerrors block at least reported all errors so that people didn't need to override this block just to see their custom errors.
There was a problem hiding this comment.
It would be, but there's some custom rendering behaviour for clashes which makes that a bit tricky.
There was a problem hiding this comment.
I'm guessing we could keep the special casing but still display everything else, but let's leave that for a later PR.
…idators use the same err_type string
To help address #358 , this refactors to schedule validation code to use explicit lists of validation functions.
This should make it much simpler to add custom validators to extend the behaviour of the schedule.