Checkbox

Merge lp://staging/~kissiel/checkbox/validation-rework into lp://staging/checkbox

validation-rework
Merge into trunk

Proposed by Maciej Kisielewski on 2016-09-28

Status:	Merged
Approved by:	Sylvain Pineau on 2016-09-29
Approved revision:	4514
Merged at revision:	4511
Proposed branch:	lp://staging/~kissiel/checkbox/validation-rework
Merge into:	lp://staging/checkbox
Diff against target:	969 lines (+222/-242) 12 files modified plainbox/plainbox/impl/unit/category.py (+6/-22) plainbox/plainbox/impl/unit/concrete_validators.py (+57/-0) plainbox/plainbox/impl/unit/exporter.py (+12/-22) plainbox/plainbox/impl/unit/file.py (+2/-7) plainbox/plainbox/impl/unit/job.py (+48/-72) plainbox/plainbox/impl/unit/manifest.py (+11/-14) plainbox/plainbox/impl/unit/packaging.py (+6/-6) plainbox/plainbox/impl/unit/template.py (+8/-8) plainbox/plainbox/impl/unit/testplan.py (+21/-32) plainbox/plainbox/impl/unit/unit.py (+14/-47) plainbox/plainbox/impl/unit/unit_with_id.py (+7/-12) plainbox/plainbox/impl/unit/validators.py (+30/-0)
To merge this branch:	bzr merge lp://staging/~kissiel/checkbox/validation-rework
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Sylvain Pineau (community)		2016-09-28	Approve on 2016-09-29
Review via email: mp+307082@code.staging.launchpad.net

Description of the change

This MR refactors validation a bit. Three goals achieved here:

1) readability improvements (my opinion)
2) better performance (every plainbox invocation runs validation, the code on this branch runs a few percent faster)
3) less code!
4) more in line with zen of python

Important change here is what field_validators are. Previously this could have been an instance of a validator, a type of a validator or list of any of the above. This branch makes it instance-only.

In terms of testing, there should be no tangible changes in how *boxes work.

Revision history for this message

Sylvain Pineau (sylvain-pineau) wrote on 2016-09-29:

Great stuff!

Just one question, see below, but applicable to all remaining PresentFieldValidator(severity=Severity.advice).

review: Needs Information

Revision history for this message

Maciej Kisielewski (kissiel) wrote on 2016-09-29:

This way present() would have to be a function yielding a concrete_validator (a constructor, really), so I wanted to stick to explicit and very oop-clear rules.
Right?

Revision history for this message

Sylvain Pineau (sylvain-pineau) wrote on 2016-09-29:

It's ok and I like the new syntax a lot. It's finally good to have the old way in a few places in case we want to change severity (as a example). But getting rid of all those lambdas is a bug win.

review: Approve

Revision history for this message

Sylvain Pineau (sylvain-pineau) wrote on 2016-09-29:

s/bug/big hehe