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

[Maciej Kisielewski]

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.

[Sylvain Pineau (sylvain-pineau)]

Great stuff!

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

review: Needs Information

[Maciej Kisielewski (kissiel)]

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?

[Sylvain Pineau (sylvain-pineau)]

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.

+1

review: Approve

[Sylvain Pineau (sylvain-pineau)]

s/bug/big hehe

=== modified file 'plainbox/plainbox/impl/unit/category.py'
--- plainbox/plainbox/impl/unit/category.py	2016-05-11 14:36:13 +0000
+++ plainbox/plainbox/impl/unit/category.py	2016-09-28 20:21:08 +0000
@@ -30,16 +30,10 @@
 
 import logging
 
-from plainbox.i18n import gettext as _
 from plainbox.i18n import gettext_noop as N_
 from plainbox.impl.symbol import SymbolDef
+from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit.unit_with_id import UnitWithId
-from plainbox.impl.unit.validators import CorrectFieldValueValidator
-from plainbox.impl.unit.validators import PresentFieldValidator
-from plainbox.impl.unit.validators import TemplateVariantFieldValidator
-from plainbox.impl.unit.validators import TranslatableFieldValidator
-from plainbox.impl.validation import Problem
-from plainbox.impl.validation import Severity
 
 __all__ = ['CategoryUnit']
 
@@ -108,20 +102,10 @@
 
         field_validators = {
             fields.name: [
-                TranslatableFieldValidator,
-                TemplateVariantFieldValidator,
-                PresentFieldValidator,
-                # We want the name to be a single line
-                CorrectFieldValueValidator(
-                    lambda name: name.count("\n") == 0,
-                    Problem.wrong, Severity.warning,
-                    message=_("please use only one line"),
-                    onlyif=lambda unit: unit.name is not None),
-                # We want the name to be relatively short
-                CorrectFieldValueValidator(
-                    lambda name: len(name) <= 80,
-                    Problem.wrong, Severity.warning,
-                    message=_("please stay under 80 characters"),
-                    onlyif=lambda unit: unit.name is not None),
+                concrete_validators.translatable,
+                concrete_validators.templateVariant,
+                concrete_validators.present,
+                concrete_validators.oneLine,
+                concrete_validators.shortValue,
             ]
         }

=== added file 'plainbox/plainbox/impl/unit/concrete_validators.py'
--- plainbox/plainbox/impl/unit/concrete_validators.py	1970-01-01 00:00:00 +0000
+++ plainbox/plainbox/impl/unit/concrete_validators.py	2016-09-28 20:21:08 +0000
@@ -0,0 +1,57 @@
+# This file is part of Checkbox.
+#
+# Copyright 2016 Canonical Ltd.
+# Written by:
+#   Maciej Kisielewski <[e-mail]>
+#
+# Checkbox is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3,
+# as published by the Free Software Foundation.
+#
+# Checkbox is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
+"""
+:mod:`plainbox.impl.unit.concrete_validators` -- common validator instances
+===========================================================================
+
+This module gathers common validator instances that can be shared among
+multiple unit types as their field_validators.
+"""
+
+from plainbox.i18n import gettext as _
+from plainbox.impl.unit.validators import CorrectFieldValueValidator
+from plainbox.impl.validation import Problem
+from plainbox.impl.validation import Severity
+
+from plainbox.impl.unit.validators import PresentFieldValidator
+from plainbox.impl.unit.validators import TemplateInvariantFieldValidator
+from plainbox.impl.unit.validators import TemplateVariantFieldValidator
+from plainbox.impl.unit.validators import TranslatableFieldValidator
+from plainbox.impl.unit.validators import UntranslatableFieldValidator
+
+
+translatable = TranslatableFieldValidator()
+templateVariant = TemplateVariantFieldValidator()
+templateInvariant = TemplateInvariantFieldValidator()
+untranslatable = UntranslatableFieldValidator()
+present = PresentFieldValidator()
+
+localDeprecated = CorrectFieldValueValidator(
+    lambda plugin: plugin != 'local', Problem.deprecated, Severity.advice,
+    message=_("please migrate to job templates, see plainbox-template-unit(7)"
+              " for details"))
+
+oneLine = CorrectFieldValueValidator(
+    lambda field: field is not None and field.count("\n") == 0,
+    Problem.wrong, Severity.warning,
+    message=_("please use only one line"))
+
+shortValue = CorrectFieldValueValidator(
+    lambda field: field is not None and len(field) <= 80,
+    Problem.wrong, Severity.warning,
+    message=_("please stay under 80 characters"))

=== modified file 'plainbox/plainbox/impl/unit/exporter.py'
--- plainbox/plainbox/impl/unit/exporter.py	2015-06-25 11:52:05 +0000
+++ plainbox/plainbox/impl/unit/exporter.py	2016-09-28 20:21:08 +0000
@@ -1,8 +1,9 @@
 # This file is part of Checkbox.
 #
-# Copyright 2015 Canonical Ltd.
+# Copyright 2015-2016 Canonical Ltd.
 # Written by:
 #   Sylvain Pineau <[e-mail]>
+#   Maciej Kisielewski <[e-mail]>
 #
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
@@ -26,11 +27,10 @@
 
 from plainbox.i18n import gettext as _
 from plainbox.impl.symbol import SymbolDef
+from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit.unit_with_id import UnitWithId
 from plainbox.impl.unit.validators import CorrectFieldValueValidator
 from plainbox.impl.unit.validators import PresentFieldValidator
-from plainbox.impl.unit.validators import TranslatableFieldValidator
-from plainbox.impl.unit.validators import UntranslatableFieldValidator
 from plainbox.impl.validation import Problem
 from plainbox.impl.validation import Severity
 
@@ -115,40 +115,30 @@
         field_validators = {
             fields.summary: [
                 PresentFieldValidator(severity=Severity.advice),
-                TranslatableFieldValidator,
-                # We want the summary to be a single line
-                CorrectFieldValueValidator(
-                    lambda summary: summary.count("\n") == 0,
-                    Problem.wrong, Severity.warning,
-                    message=_("please use only one line"),
-                    onlyif=lambda unit: unit.summary is not None),
-                # We want the summary to be relatively short
-                CorrectFieldValueValidator(
-                    lambda summary: len(summary) <= 80,
-                    Problem.wrong, Severity.warning,
-                    message=_("please stay under 80 characters"),
-                    onlyif=lambda unit: unit.summary is not None),
+                concrete_validators.translatable,
+                concrete_validators.oneLine,
+                concrete_validators.shortValue,
             ],
             fields.entry_point: [
-                PresentFieldValidator,
-                UntranslatableFieldValidator,
+                concrete_validators.present,
+                concrete_validators.untranslatable,
                 CorrectFieldValueValidator(
                     lambda entry_point: pkg_resources.load_entry_point(
                         'plainbox', 'plainbox.exporter', entry_point),
                     Problem.wrong, Severity.error),
             ],
             fields.file_extension: [
-                PresentFieldValidator,
-                UntranslatableFieldValidator,
+                concrete_validators.present,
+                concrete_validators.untranslatable,
                 CorrectFieldValueValidator(
                     lambda extension: re.search("^[\w\.\-]+$", extension),
                     Problem.syntax_error, Severity.error),
             ],
             fields.options: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
             ],
             fields.data: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
                 CorrectFieldValueValidator(
                     lambda value, unit: json.loads(value),
                     Problem.syntax_error, Severity.error,

=== modified file 'plainbox/plainbox/impl/unit/file.py'
--- plainbox/plainbox/impl/unit/file.py	2015-02-06 15:30:54 +0000
+++ plainbox/plainbox/impl/unit/file.py	2016-09-28 20:21:08 +0000
@@ -30,6 +30,7 @@
 from plainbox.impl.unit.job import propertywithsymbols
 from plainbox.impl.unit.unit import Unit, UnitValidator
 from plainbox.impl.unit.validators import CorrectFieldValueValidator
+from plainbox.impl.unit.validators import MemberOfFieldValidator
 from plainbox.impl.validation import Problem
 from plainbox.impl.validation import Severity
 
@@ -146,11 +147,5 @@
                         'faq.html#faq-1'
                     )),
             ],
-            fields.role: [
-                CorrectFieldValueValidator(
-                    lambda value: value in FileRole.get_all_symbols(),
-                    message=_('valid values are: {}').format(
-                        ', '.join(str(sym) for sym in sorted(
-                            FileRole.get_all_symbols())))),
-            ]
+            fields.role: [MemberOfFieldValidator(FileRole.get_all_symbols())]
         }

=== modified file 'plainbox/plainbox/impl/unit/job.py'
--- plainbox/plainbox/impl/unit/job.py	2016-09-22 18:53:53 +0000
+++ plainbox/plainbox/impl/unit/job.py	2016-09-28 20:21:08 +0000
@@ -1,9 +1,10 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012-2014 Canonical Ltd.
+# Copyright 2012-2016 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
 #   Sylvain Pineau <[e-mail]>
+#   Maciej Kisielewski <[e-mail]>
 #
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
@@ -34,18 +35,17 @@
 from plainbox.impl.secure.origin import JobOutputTextSource
 from plainbox.impl.secure.origin import Origin
 from plainbox.impl.symbol import SymbolDef
+from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit.unit_with_id import UnitWithId
 from plainbox.impl.unit.validators import CorrectFieldValueValidator
 from plainbox.impl.unit.validators import DeprecatedFieldValidator
+from plainbox.impl.unit.validators import MemberOfFieldValidator
 from plainbox.impl.unit.validators import PresentFieldValidator
 from plainbox.impl.unit.validators import ReferenceConstraint
 from plainbox.impl.unit.validators import ShellProgramValidator
-from plainbox.impl.unit.validators import TemplateInvariantFieldValidator
-from plainbox.impl.unit.validators import TemplateVariantFieldValidator
-from plainbox.impl.unit.validators import TranslatableFieldValidator
 from plainbox.impl.unit.validators import UnitReferenceValidator
-from plainbox.impl.unit.validators import UntranslatableFieldValidator
 from plainbox.impl.unit.validators import UselessFieldValidator
+
 from plainbox.impl.validation import Problem
 from plainbox.impl.validation import Severity
 from plainbox.impl.xparsers import Error
@@ -700,51 +700,32 @@
 
         field_validators = {
             fields.name: [
-                UntranslatableFieldValidator,
-                TemplateVariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateVariant,
                 DeprecatedFieldValidator(
                     _("use 'id' and 'summary' instead of 'name'")),
             ],
             # NOTE: 'id' validators are "inherited" so we don't have it here
             fields.summary: [
-                TranslatableFieldValidator,
-                TemplateVariantFieldValidator,
+                concrete_validators.translatable,
+                concrete_validators.templateVariant,
                 PresentFieldValidator(severity=Severity.advice),
-                # We want the summary to be a single line
-                CorrectFieldValueValidator(
-                    lambda summary: summary.count("\n") == 0,
-                    Problem.wrong, Severity.warning,
-                    message=_("please use only one line"),
-                    onlyif=lambda unit: unit.summary is not None),
-                # We want the summary to be relatively short
-                CorrectFieldValueValidator(
-                    lambda summary: len(summary) <= 80,
-                    Problem.wrong, Severity.warning,
-                    message=_("please stay under 80 characters"),
-                    onlyif=lambda unit: unit.summary is not None),
+                concrete_validators.oneLine,
+                concrete_validators.shortValue,
             ],
             fields.plugin: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
-                PresentFieldValidator,
-                CorrectFieldValueValidator(
-                    lambda plugin: (
-                        plugin in JobDefinition.plugin.get_all_symbols()),
-                    message=_('valid values are: {}').format(
-                        ', '.join(str(sym) for sym in sorted(
-                            _PluginValues.get_all_symbols())))),
-                CorrectFieldValueValidator(
-                    lambda plugin: plugin != 'local',
-                    Problem.deprecated, Severity.advice,
-                    message=_("please migrate to job templates, "
-                              "see plainbox-template-unit(7) for details")),
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
+                concrete_validators.present,
+                MemberOfFieldValidator(_PluginValues.get_all_symbols()),
+                concrete_validators.localDeprecated,
                 CorrectFieldValueValidator(
                     lambda plugin: plugin != 'user-verify',
                     Problem.deprecated, Severity.advice,
                     message=_("please migrate to user-interact-verify")),
             ],
             fields.command: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
                 # All jobs except for manual must have a command
                 PresentFieldValidator(
                     message=_("command is mandatory for non-manual jobs"),
@@ -768,11 +749,11 @@
                               " instead of CHECKBOX_DATA"),
                     onlyif=lambda unit: unit.command is not None),
                 # We want to catch silly mistakes that shlex can detect
-                ShellProgramValidator,
+                ShellProgramValidator(),
             ],
             fields.description: [
-                TranslatableFieldValidator,
-                TemplateVariantFieldValidator,
+                concrete_validators.translatable,
+                concrete_validators.templateVariant,
                 # Description is mandatory for manual jobs
                 PresentFieldValidator(
                     message=_("manual jobs must have a description field, or a"
@@ -796,7 +777,7 @@
                             unit.verification is None))),
             ],
             fields.purpose: [
-                TranslatableFieldValidator,
+                concrete_validators.translatable,
                 PresentFieldValidator(
                     severity=Severity.advice,
                     message=("please use purpose, steps, and verification"
@@ -807,7 +788,7 @@
                     unit.get_record_value('summary') is None),
             ],
             fields.steps: [
-                TranslatableFieldValidator,
+                concrete_validators.translatable,
                 PresentFieldValidator(
                     severity=Severity.advice,
                     message=("please use purpose, steps, and verification"
@@ -817,7 +798,7 @@
                     unit.startup_user_interaction_required),
             ],
             fields.verification: [
-                TranslatableFieldValidator,
+                concrete_validators.translatable,
                 PresentFieldValidator(
                     severity=Severity.advice,
                     message=("please use purpose, steps, and verification"
@@ -827,8 +808,8 @@
                         'manual', 'user-verify', 'user-interact-verify')),
             ],
             fields.user: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 # User should be either None or 'root'
                 CorrectFieldValueValidator(
                     message=_("user can only be 'root'"),
@@ -839,16 +820,16 @@
                     onlyif=lambda unit: unit.command is None)
             ],
             fields.environ: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 # Environ is useless without a command to run
                 UselessFieldValidator(
                     message=_("environ without a command makes no sense"),
                     onlyif=lambda unit: unit.command is None),
             ],
             fields.estimated_duration: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 PresentFieldValidator(
                     severity=Severity.advice,
                     onlyif=lambda unit: 'simple' not in unit.get_flag_set()
@@ -860,7 +841,7 @@
                         unit.get_record_value('estimated_duration'))),
             ],
             fields.depends: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
                 CorrectFieldValueValidator(
                     lambda value, unit: (
                         unit.get_direct_dependencies() is not None)),
@@ -874,7 +855,7 @@
                 #       onlyif job itself is not deprecated
             ],
             fields.after: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
                 CorrectFieldValueValidator(
                     lambda value, unit: (
                         unit.get_after_dependencies() is not None)),
@@ -886,7 +867,7 @@
                             message=_("the referenced unit is not a job"))])
             ],
             fields.requires: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
                 CorrectFieldValueValidator(
                     lambda value, unit: unit.get_resource_program(),
                     onlyif=lambda unit: unit.requires is not None),
@@ -908,16 +889,16 @@
                 #       onlyif job itself is not deprecated
             ],
             fields.shell: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 # Shell should be only '/bin/sh', or None (which gives bash)
-                CorrectFieldValueValidator(
-                    lambda shell: shell in ('/bin/sh', '/bin/bash', 'bash'),
+                MemberOfFieldValidator(
+                    ['/bin/sh', '/bin/bash', 'bash'],
                     message=_("only /bin/sh and /bin/bash are allowed")),
             ],
             fields.imports: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 CorrectFieldValueValidator(
                     lambda value, unit: (
                         list(unit.get_imported_jobs()) is not None)),
@@ -933,8 +914,8 @@
                 #       onlyif job itself is not deprecated
             ],
             fields.category_id: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 UnitReferenceValidator(
                     lambda unit: (
                         [unit.get_category_id()] if unit.category_id else ()),
@@ -948,8 +929,8 @@
                 #       onlyif job itself is not deprecated
             ],
             fields.flags: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 CorrectFieldValueValidator(
                     lambda value, unit: (
                         'simple' in unit.get_flag_set() or
@@ -999,8 +980,8 @@
                     onlyif=lambda unit: unit.command is None),
             ],
             fields.qml_file: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 PresentFieldValidator(
                     onlyif=lambda unit: unit.plugin == 'qml'),
                 CorrectFieldValueValidator(
@@ -1016,14 +997,9 @@
                                          unit.qml_file)),
             ],
             fields.certification_status: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
-                CorrectFieldValueValidator(
-                    lambda certification_status: (
-                        certification_status in
-                        _CertificationStatusValues.get_all_symbols()),
-                    message=_('valid values are: {}').format(
-                        ', '.join(str(sym) for sym in sorted(
-                            _CertificationStatusValues.get_all_symbols())))),
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
+                MemberOfFieldValidator(
+                    _CertificationStatusValues.get_all_symbols()),
             ],
         }

=== modified file 'plainbox/plainbox/impl/unit/manifest.py'
--- plainbox/plainbox/impl/unit/manifest.py	2015-03-30 17:35:30 +0000
+++ plainbox/plainbox/impl/unit/manifest.py	2016-09-28 20:21:08 +0000
@@ -1,8 +1,9 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012-2015 Canonical Ltd.
+# Copyright 2012-2016 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
+#   Maciej Kisielewski <[e-mail]>
 #
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
@@ -20,12 +21,9 @@
 import logging
 
 from plainbox.impl.symbol import SymbolDef
+from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit.unit_with_id import UnitWithId
-from plainbox.impl.unit.validators import CorrectFieldValueValidator
-from plainbox.impl.unit.validators import PresentFieldValidator
-from plainbox.impl.unit.validators import TemplateVariantFieldValidator
-from plainbox.impl.unit.validators import TranslatableFieldValidator
-from plainbox.impl.unit.validators import UntranslatableFieldValidator
+from plainbox.impl.unit.validators import MemberOfFieldValidator
 
 logger = logging.getLogger("plainbox.unit.manifest")
 
@@ -98,20 +96,19 @@
 
         field_validators = {
             fields.name: [
-                TranslatableFieldValidator,
-                TemplateVariantFieldValidator,
-                PresentFieldValidator,
+                concrete_validators.translatable,
+                concrete_validators.templateVariant,
+                concrete_validators.present,
             ],
             fields.value_type: [
-                UntranslatableFieldValidator,
-                PresentFieldValidator(),
-                CorrectFieldValueValidator(
-                    lambda value_type: value_type in ('bool', 'natural')),
+                concrete_validators.untranslatable,
+                concrete_validators.present,
+                MemberOfFieldValidator(['bool', 'natural']),
             ],
             fields.value_unit: [
                 # OPTIONAL
             ],
             fields.resource_key: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable
             ]
         }

=== modified file 'plainbox/plainbox/impl/unit/packaging.py'
--- plainbox/plainbox/impl/unit/packaging.py	2016-05-20 13:54:59 +0000
+++ plainbox/plainbox/impl/unit/packaging.py	2016-09-28 20:21:08 +0000
@@ -1,8 +1,9 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012-2015 Canonical Ltd.
+# Copyright 2012-2016 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
+#   Maciej Kisielewski <[e-mail]>
 #
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
@@ -120,9 +121,8 @@
 from plainbox.i18n import gettext as _
 from plainbox.impl.device import get_os_release
 from plainbox.impl.symbol import SymbolDef
+from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit.unit import Unit
-from plainbox.impl.unit.validators import PresentFieldValidator
-from plainbox.impl.unit.validators import UntranslatableFieldValidator
 
 _logger = logging.getLogger("plainbox.unit.packaging")
 
@@ -164,11 +164,11 @@
 
         field_validators = {
             fields.os_id: [
-                UntranslatableFieldValidator,
-                PresentFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.present,
             ],
             fields.os_version_id: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
             ],
         }
 

=== modified file 'plainbox/plainbox/impl/unit/template.py'
--- plainbox/plainbox/impl/unit/template.py	2016-05-11 14:36:13 +0000
+++ plainbox/plainbox/impl/unit/template.py	2016-09-28 20:21:08 +0000
@@ -1,8 +1,9 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012-2014 Canonical Ltd.
+# Copyright 2012-2016 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
+#   Maciej Kisielewski <[e-mail]>
 #
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
@@ -32,14 +33,13 @@
 from plainbox.impl.secure.origin import Origin
 from plainbox.impl.symbol import SymbolDef
 from plainbox.impl.unit import all_units
+from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit import get_accessed_parameters
 from plainbox.impl.unit.unit import Unit
 from plainbox.impl.unit.unit import UnitValidator
 from plainbox.impl.unit.validators import CorrectFieldValueValidator
-from plainbox.impl.unit.validators import PresentFieldValidator
 from plainbox.impl.unit.validators import ReferenceConstraint
 from plainbox.impl.unit.validators import UnitReferenceValidator
-from plainbox.impl.unit.validators import UntranslatableFieldValidator
 from plainbox.impl.validation import Problem
 from plainbox.impl.validation import Severity
 
@@ -427,7 +427,7 @@
 
         field_validators = {
             fields.template_unit: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
                 CorrectFieldValueValidator(
                     lambda value, unit: (
                         unit.get_record_value('template-unit') is not None),
@@ -436,8 +436,8 @@
                         " unit type")),
             ],
             fields.template_resource: [
-                UntranslatableFieldValidator,
-                PresentFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.present,
                 UnitReferenceValidator(
                     lambda unit: (
                         [unit.resource_id] if unit.resource_id else []),
@@ -457,7 +457,7 @@
                 #       onlyif job itself is not deprecated
             ],
             fields.template_filter: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
                 # All templates need a valid (or empty) template filter
                 CorrectFieldValueValidator(
                     lambda value, unit: unit.get_filter_program(),
@@ -465,7 +465,7 @@
                 # TODO: must refer to the same job as template-resource
             ],
             fields.template_imports: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
                 CorrectFieldValueValidator(
                     lambda value, unit: (
                         list(unit.get_imported_jobs()) is not None)),

=== modified file 'plainbox/plainbox/impl/unit/testplan.py'
--- plainbox/plainbox/impl/unit/testplan.py	2016-06-09 20:12:35 +0000
+++ plainbox/plainbox/impl/unit/testplan.py	2016-09-28 20:21:08 +0000
@@ -1,8 +1,9 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012-2014 Canonical Ltd.
+# Copyright 2012-2016 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
+#   Maciej Kisielewski <[e-mail]>
 #
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
@@ -32,16 +33,14 @@
 from plainbox.impl.secure.qualifiers import OperatorMatcher
 from plainbox.impl.secure.qualifiers import PatternMatcher
 from plainbox.impl.symbol import SymbolDef
+from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit.unit_with_id import UnitWithId
 from plainbox.impl.unit.validators import CorrectFieldValueValidator
 from plainbox.impl.unit.validators import FieldValidatorBase
 from plainbox.impl.unit.validators import PresentFieldValidator
 from plainbox.impl.unit.validators import ReferenceConstraint
 from plainbox.impl.unit.validators import TemplateInvariantFieldValidator
-from plainbox.impl.unit.validators import TemplateVariantFieldValidator
-from plainbox.impl.unit.validators import TranslatableFieldValidator
 from plainbox.impl.unit.validators import UnitReferenceValidator
-from plainbox.impl.unit.validators import UntranslatableFieldValidator
 from plainbox.impl.unit.validators import compute_value_map
 from plainbox.impl.validation import Problem
 from plainbox.impl.validation import Severity
@@ -609,40 +608,30 @@
 
         field_validators = {
             fields.name: [
-                TranslatableFieldValidator,
-                TemplateVariantFieldValidator,
-                PresentFieldValidator,
-                # We want the summary to be a single line
-                CorrectFieldValueValidator(
-                    lambda name: name.count("\n") == 0,
-                    Problem.wrong, Severity.warning,
-                    message=_("please use only one line"),
-                    onlyif=lambda unit: unit.name is not None),
-                # We want the summary to be relatively short
-                CorrectFieldValueValidator(
-                    lambda name: len(name) <= 80,
-                    Problem.wrong, Severity.warning,
-                    message=_("please stay under 80 characters"),
-                    onlyif=lambda unit: unit.name is not None),
+                concrete_validators.translatable,
+                concrete_validators.templateVariant,
+                concrete_validators.present,
+                concrete_validators.oneLine,
+                concrete_validators.shortValue,
             ],
             fields.description: [
-                TranslatableFieldValidator,
-                TemplateVariantFieldValidator,
+                concrete_validators.translatable,
+                concrete_validators.templateVariant,
                 PresentFieldValidator(
                     severity=Severity.advice,
                     onlyif=lambda unit: unit.virtual is False),
             ],
             fields.include: [
-                NonEmptyPatternIntersectionValidator,
-                PresentFieldValidator(),
+                NonEmptyPatternIntersectionValidator(),
+                concrete_validators.present,
             ],
             fields.mandatory_include: [
-                NonEmptyPatternIntersectionValidator,
-                NoBaseIncludeValidator,
+                NonEmptyPatternIntersectionValidator(),
+                NoBaseIncludeValidator(),
             ],
             fields.bootstrap_include: [
-                UntranslatableFieldValidator,
-                NoBaseIncludeValidator,
+                concrete_validators.untranslatable,
+                NoBaseIncludeValidator(),
                 UnitReferenceValidator(
                     lambda unit: unit.get_bootstrap_job_ids(),
                     constraints=[
@@ -656,14 +645,14 @@
                                       "allowed in bootstrapping_include"))])
             ],
             fields.exclude: [
-                NonEmptyPatternIntersectionValidator,
+                NonEmptyPatternIntersectionValidator(),
             ],
             fields.nested_part: [
-                NonEmptyPatternIntersectionValidator,
+                NonEmptyPatternIntersectionValidator(),
             ],
             fields.estimated_duration: [
-                UntranslatableFieldValidator,
-                TemplateInvariantFieldValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 PresentFieldValidator(
                     severity=Severity.advice,
                     onlyif=lambda unit: unit.virtual is False),
@@ -675,7 +664,7 @@
                         unit.get_record_value('estimated_duration'))),
             ],
             fields.icon: [
-                UntranslatableFieldValidator,
+                concrete_validators.untranslatable,
             ],
             fields.category_overrides: [
                 # optional

=== modified file 'plainbox/plainbox/impl/unit/unit.py'
--- plainbox/plainbox/impl/unit/unit.py	2016-05-20 13:54:59 +0000
+++ plainbox/plainbox/impl/unit/unit.py	2016-09-28 20:21:08 +0000
@@ -1,8 +1,9 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012-2014 Canonical Ltd.
+# Copyright 2012-2016 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
+#   Maciej Kisielewski <[e-mail]>
 #
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
@@ -35,13 +36,12 @@
 from plainbox.impl.symbol import SymbolDef
 from plainbox.impl.symbol import SymbolDefMeta
 from plainbox.impl.symbol import SymbolDefNs
+from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit import get_accessed_parameters
 from plainbox.impl.unit.validators import IFieldValidator
 from plainbox.impl.unit.validators import MultiUnitFieldIssue
 from plainbox.impl.unit.validators import PresentFieldValidator
-from plainbox.impl.unit.validators import TemplateInvariantFieldValidator
 from plainbox.impl.unit.validators import UnitFieldIssue
-from plainbox.impl.unit.validators import UntranslatableFieldValidator
 from plainbox.impl.validation import Problem
 from plainbox.impl.validation import Severity
 
@@ -77,9 +77,10 @@
         :returns:
             A generator yielding subsequent issues
         """
-        for field_validator, field in self.make_field_validators(unit):
-            for issue in field_validator.check(self, unit, field):
-                yield issue
+        for field, validators in sorted(unit.Meta.field_validators.items()):
+            for validator in validators:
+                for issue in validator.check(self, unit, field):
+                    yield issue
 
     def check_in_context(self, unit, context):
         """
@@ -92,41 +93,11 @@
         :returns:
             A generator yielding subsequent issues
         """
-        for field_validator, field in self.make_field_validators(unit):
-            for issue in field_validator.check_in_context(
-                    self, unit, field, context):
-                yield issue
-
-    def make_field_validators(self, unit):
-        """
-        Convert unit meta-data to a sequence of validators
-
-        :returns:
-            A generator for pairs (field_validator, field) where
-            field_validator is an instance of :class:`IFieldValidator` and
-            field is a symbol with the field name.
-        """
-        for field, spec in sorted(unit.Meta.field_validators.items()):
-            if isinstance(spec, type):
-                validator_list = [spec]
-            elif isinstance(spec, list):
-                validator_list = spec
-            else:
-                raise TypeError(_(
-                    "{}.Meta.fields[{!r}] is not a validator"
-                ).format(unit.__class__.__name__, field))
-            for index, spec in enumerate(validator_list):
-                # If it's a validator class, instantiate it
-                if isinstance(spec, type) \
-                        and issubclass(spec, IFieldValidator):
-                    yield spec(), field
-                # If it's a validator instance, just return it
-                elif isinstance(spec, IFieldValidator):
-                    yield spec, field
-                else:
-                    raise TypeError(_(
-                        "{}.Meta.fields[{!r}][{}] is not a validator"
-                    ).format(unit.__class__.__name__, field, index))
+        for field, validators in sorted(unit.Meta.field_validators.items()):
+            for validator in validators:
+                for issue in validator.check_in_context(
+                        self, unit, field, context):
+                    yield issue
 
     def advice(self, unit, field, kind, message=None, *, offset=0,
                origin=None):
@@ -840,12 +811,8 @@
 
         field_validators = {
             fields.unit: [
-                # We don't want anyone marking unit type up for translation
-                UntranslatableFieldValidator,
-                # We want each instantiated template to define same unit type
-                TemplateInvariantFieldValidator,
-                # We want to gently advise everyone to mark all units with
-                # and explicit unit type so that we can disable default 'job'
+                concrete_validators.untranslatable,
+                concrete_validators.templateInvariant,
                 PresentFieldValidator(
                     severity=Severity.advice,
                     message=_("unit should explicitly define its type")),

=== modified file 'plainbox/plainbox/impl/unit/unit_with_id.py'
--- plainbox/plainbox/impl/unit/unit_with_id.py	2016-05-11 14:36:13 +0000
+++ plainbox/plainbox/impl/unit/unit_with_id.py	2016-09-28 20:21:08 +0000
@@ -1,8 +1,9 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012-2014 Canonical Ltd.
+# Copyright 2012-2016 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
+#   Maciej Kisielewski <[e-mail]>
 #
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
@@ -26,13 +27,11 @@
 from plainbox.i18n import gettext as _
 from plainbox.i18n import gettext_noop as N_
 from plainbox.impl.symbol import SymbolDef
+from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit.unit import Unit
 from plainbox.impl.unit.unit import UnitValidator
 from plainbox.impl.unit.validators import CorrectFieldValueValidator
-from plainbox.impl.unit.validators import PresentFieldValidator
-from plainbox.impl.unit.validators import TemplateVariantFieldValidator
 from plainbox.impl.unit.validators import UniqueValueValidator
-from plainbox.impl.unit.validators import UntranslatableFieldValidator
 
 __all__ = ['UnitWithId']
 
@@ -111,14 +110,10 @@
 
         field_validators = {
             fields.id: [
-                # We don't want anyone marking id up for translation
-                UntranslatableFieldValidator,
-                # We want this field to be present at all times
-                PresentFieldValidator,
-                # We want each instance to have a different identifier
-                TemplateVariantFieldValidator,
-                # When checking in a globally, all units need an unique value
-                UniqueValueValidator,
+                concrete_validators.untranslatable,
+                concrete_validators.present,
+                concrete_validators.templateVariant,
+                UniqueValueValidator(),
                 # We want to have bare, namespace-less identifiers
                 CorrectFieldValueValidator(
                     lambda value, unit: (

=== modified file 'plainbox/plainbox/impl/unit/validators.py'
--- plainbox/plainbox/impl/unit/validators.py	2016-05-20 13:54:59 +0000
+++ plainbox/plainbox/impl/unit/validators.py	2016-09-28 20:21:08 +0000
@@ -364,6 +364,36 @@
         super().__init__(correct_fn, kind, severity, message, onlyif)
 
 
+class MemberOfFieldValidator(CorrectFieldValueValidator):
+    """Validator ensuring the value is a member of a given set."""
+
+    def __init__(self, possible_values, kind=None, severity=None, message=None,
+                 onlyif=None):
+        """
+        possible_values:
+            Iterable with values that the membership will be testsed against.
+        kind:
+            Kind of issue to report. By default this is Problem.useless
+        severity:
+            Severity of the issue to report. By default this is
+            Severity.warning
+        message:
+            Customized error message. This message will be used to report the
+            issue if the validation fails. By default it is derived from the
+            specified issue ``kind`` by :meth:`UnitValidator.explain()`.
+        onlyif:
+            An optional function that checks if this validator should be
+            applied or not. The function is called with the `unit` as the only
+            argument.  If it returns True then the validator proceeds to
+            perform its check.
+        """
+        def correct_fn(value): return value in possible_values
+        if not message:
+            message = _('valid values are: {}').format(
+                ', '.join(str(val) for val in sorted(possible_values)))
+        super().__init__(correct_fn, kind, severity, message, onlyif)
+
+
 class DeprecatedFieldValidator(FieldValidatorBase):
     """
     Validator ensuring that deprecated field is not used (passed a value)