better error messages

test/test_option_validator.py

@@ -11,15 +11,18 @@ from tiramisu.error import ValueWarning
 from tiramisu.i18n import _
 
 
+msg_err = _("attention, {0} could be an invalid {1} for option {2}, {3}")
+
+
 def return_true(value, param=None):
     if value == 'val' and param in [None, 'yes']:
         return True
-    return ValueError('error')
+    return ValueError('test error')
 
 
 def return_false(value, param=None):
     if value == 'val' and param in [None, 'yes']:
-        return ValueError('error')
+        return ValueError('test error')
 
 
 def return_val(value, param=None):
@@ -28,7 +31,7 @@ def return_val(value, param=None):
 
 def return_if_val(value):
     if value != 'val':
-        return ValueError('error')
+        return ValueError('test error')
 
 
 def test_validator():
@@ -96,7 +99,7 @@ def test_validator_warning():
         cfg.opt2 = 'val'
     assert len(w) == 1
     assert w[0].message.opt == opt2
-    assert str(w[0].message) == _("warning on the value of the option {0}: {1}").format('opt2', 'error')
+    assert str(w[0].message) == msg_err.format('val', opt2.display_name, 'opt2', 'test error')
     #
     with warnings.catch_warnings(record=True) as w:
         cfg.opt3.append('val')
@@ -106,7 +109,7 @@ def test_validator_warning():
         cfg.opt3.append('val1')
     assert len(w) == 1
     assert w[0].message.opt == opt3
-    assert str(w[0].message) == _("warning on the value of the option {0}: {1}").format('opt3', 'error')
+    assert str(w[0].message) == msg_err.format('val1', opt3.display_name, 'opt3', 'test error')
     raises(ValueError, "cfg.opt2 = 1")
     #
     with warnings.catch_warnings(record=True) as w:
@@ -114,9 +117,9 @@ def test_validator_warning():
         cfg.opt3.append('val')
     assert len(w) == 2
     assert w[0].message.opt == opt2
-    assert str(w[0].message) == _("warning on the value of the option {0}: {1}").format('opt2', 'error')
+    assert str(w[0].message) == msg_err.format('val', opt2.display_name, 'opt2', 'test error')
     assert w[1].message.opt == opt3
-    assert str(w[1].message) == _("warning on the value of the option {0}: {1}").format('opt3', 'error')
+    assert str(w[0].message) == msg_err.format('val', opt2.display_name, 'opt2', 'test error')
 
 
 def test_validator_warning_disabled():
@@ -168,29 +171,29 @@ def test_validator_warning_master_slave():
         cfg.ip_admin_eth0.netmask_admin_eth0 = ['val1']
     assert len(w) == 1
     assert w[0].message.opt == netmask_admin_eth0
-    assert str(w[0].message) == _("warning on the value of the option {0}: {1}").format('netmask_admin_eth0', 'error')
+    assert str(w[0].message) == msg_err.format('val1', netmask_admin_eth0.display_name, 'netmask_admin_eth0', 'test error')
     #
     with warnings.catch_warnings(record=True) as w:
         cfg.ip_admin_eth0.ip_admin_eth0 = ['val']
     assert len(w) == 1
     assert w[0].message.opt == ip_admin_eth0
-    assert str(w[0].message) == _("warning on the value of the option {0}: {1}").format('ip_admin_eth0', 'error')
+    assert str(w[0].message) == msg_err.format('val', ip_admin_eth0.display_name, 'ip_admin_eth0', 'test error')
     #
     with warnings.catch_warnings(record=True) as w:
         cfg.ip_admin_eth0.ip_admin_eth0 = ['val', 'val1', 'val1']
     assert len(w) == 1
     assert w[0].message.opt == ip_admin_eth0
-    assert str(w[0].message) == _("warning on the value of the option {0}: {1}").format('ip_admin_eth0', 'error')
+    assert str(w[0].message) == msg_err.format('val', ip_admin_eth0.display_name, 'ip_admin_eth0', 'test error')
     #
     with warnings.catch_warnings(record=True) as w:
         cfg.ip_admin_eth0.ip_admin_eth0 = ['val1', 'val', 'val1']
     assert len(w) == 1
     assert w[0].message.opt == ip_admin_eth0
-    assert str(w[0].message) == _("warning on the value of the option {0}: {1}").format('ip_admin_eth0', 'error')
+    assert str(w[0].message) == msg_err.format('val', ip_admin_eth0.display_name, 'ip_admin_eth0', 'test error')
     #
     warnings.resetwarnings()
     with warnings.catch_warnings(record=True) as w:
         cfg.ip_admin_eth0.ip_admin_eth0 = ['val1', 'val1', 'val']
     assert len(w) == 1
     assert w[0].message.opt == ip_admin_eth0
-    assert str(w[0].message) == _("warning on the value of the option {0}: {1}").format('ip_admin_eth0', 'error')
+    assert str(w[0].message) == msg_err.format('val', ip_admin_eth0.display_name, 'ip_admin_eth0', 'test error')

tiramisu/option/baseoption.py

@@ -38,6 +38,13 @@ forbidden_names = frozenset(['iter_all', 'iter_group', 'find', 'find_first',
                              'read_write', 'getowner', 'set_contexts'])
 
 
+def display_list(list_):
+    if len(list_) == 1:
+        return list_[0]
+    else:
+        return ', '.join(list_[:-1]) + _(' and ') + list_[-1]
+
+
 def valid_name(name):
     "an option's name is a str and does not start with 'impl' or 'cfgimpl'"
     if not isinstance(name, str):  # pragma: optional cover
@@ -490,10 +497,11 @@ class Option(OnlyOption):
                           'submulti_index: {2}'.format(_value, _index,
                                                        submulti_index),
                           exc_info=True)
-                if '{0}'.format(err):
-                    msg = _('{0} is an invalid {1} for option {2}: {3}'
+                err_msg = '{0}'.format(err)
+                if err_msg:
+                    msg = _('{0} is an invalid {1} for option {2}, {3}'
                             '').format(_value, self.display_name,
-                                       self.impl_getname(), err)
+                                       self.impl_getname(), err_msg)
                 else:
                     msg = _('{0} is an invalid {1} for option {2}'
                             '').format(_value, self.display_name,
@@ -522,7 +530,7 @@ class Option(OnlyOption):
                     else:
                         return ret
             if warning:
-                msg = _("attention, {0} could be an invalid {1} for option {2}: {3}").format(
+                msg = _("attention, {0} could be an invalid {1} for option {2}, {3}").format(
                     _value, self.display_name, self.impl_getname(), warning)
                 if context is undefined or 'warnings' in \
                         context.cfgimpl_get_settings():
@@ -530,10 +538,11 @@ class Option(OnlyOption):
                                            ValueWarning,
                                            self.__class__.__name__, 0)
             elif error:
-                if '{0}'.format(err):
-                    msg = _("{0} is an invalid {1} for option {2}: {3}"
+                err_msg = '{0}'.format(error)
+                if err_msg:
+                    msg = _("{0} is an invalid {1} for option {2}, {3}"
                             "").format(_value, self.display_name,
-                                       self.impl_getname(), error)
+                                       self.impl_getname(), err_msg)
                 else:
                     msg = _("{0} is an invalid {1} for option {2}"
                             "").format(_value, self.display_name,
@@ -706,9 +715,9 @@ class Option(OnlyOption):
             for idx_sup, val_sup in enumerate(vals[idx_inf + 1:]):
                 if val_inf == val_sup is not None:
                     if warnings_only:
-                        msg = _("same value for {0} and {1}, should be different")
+                        msg = _("value for {0} and {1} should be different")
                     else:
-                        msg = _("same value for {0} and {1}, must be different")
+                        msg = _("value for {0} and {1} must be different")
                     log.debug('_cons_not_equal: {0} and {1} are not different'.format(val_inf, val_sup))
                     return ValueError(msg.format(opts[idx_inf].impl_getname(),
                                                  opts[idx_inf + idx_sup + 1].impl_getname()))

tiramisu/option/option.py

@@ -27,7 +27,7 @@ from ..setting import undefined
 
 from ..error import ConfigError
 from ..i18n import _
-from .baseoption import Option, validate_callback
+from .baseoption import Option, validate_callback, display_list
 from ..autolib import carry_out_calculation
 
 
@@ -100,10 +100,12 @@ class ChoiceOption(Option):
         if isinstance(values, Exception):
             return values
         if values is not undefined and not value in values:  # pragma: optional cover
-            return ValueError(_('value {0} is not permitted, '
-                                'only {1} is allowed'
-                                '').format(value,
-                                           values))
+            if len(values) == 1:
+                return ValueError(_('only {0} is allowed'
+                                    '').format(values[0]))
+            else:
+                return ValueError(_('only {0} are allowed'
+                                    '').format(display_list(values)))
 
 
 class BoolOption(Option):
@@ -114,7 +116,7 @@ class BoolOption(Option):
     def _validate(self, value, context=undefined, current_opt=undefined,
                   returns_raise=False):
         if not isinstance(value, bool):
-            return ValueError(_('invalid boolean'))  # pragma: optional cover
+            return ValueError()  # pragma: optional cover
 
 
 class IntOption(Option):
@@ -125,7 +127,7 @@ class IntOption(Option):
     def _validate(self, value, context=undefined, current_opt=undefined,
                   returns_raise=False):
         if not isinstance(value, int):
-            return ValueError(_('invalid integer'))  # pragma: optional cover
+            return ValueError()  # pragma: optional cover
 
 
 class FloatOption(Option):
@@ -136,7 +138,7 @@ class FloatOption(Option):
     def _validate(self, value, context=undefined, current_opt=undefined,
                   returns_raise=False):
         if not isinstance(value, float):
-            return ValueError(_('invalid float'))  # pragma: optional cover
+            return ValueError()  # pragma: optional cover
 
 
 class StrOption(Option):
@@ -147,7 +149,7 @@ class StrOption(Option):
     def _validate(self, value, context=undefined, current_opt=undefined,
                   returns_raise=False):
         if not isinstance(value, str):
-            return ValueError(_('invalid string'))  # pragma: optional cover
+            return ValueError()  # pragma: optional cover
 
 
 if sys.version_info[0] >= 3:  # pragma: optional cover
@@ -160,12 +162,12 @@ else:
         "represents the choice of a unicode string"
         __slots__ = tuple()
         _empty = u''
-        display_name = _('string')
+        display_name = _('unicode string')
 
         def _validate(self, value, context=undefined, current_opt=undefined,
                       returns_raise=False):
             if not isinstance(value, unicode):
-                return ValueError(_('invalid unicode'))  # pragma: optional cover
+                return ValueError()  # pragma: optional cover
 
 
 class PasswordOption(Option):
@@ -245,13 +247,11 @@ class IPOption(Option):
         ip, network, netmask = vals
         if IP(ip) not in IP('{0}/{1}'.format(network, netmask)):  # pragma: optional cover
             if warnings_only:
-                msg = _('IP {0} ({1}) not in network {2} ({3}) with netmask {4}'
-                        ' ({5})')
+                msg = _('should be in network {0}/{1} ({2}/{3})')
             else:
-                msg = _('invalid IP {0} ({1}) not in network {2} ({3}) with '
-                        'netmask {4} ({5})')
-            return ValueError(msg.format(ip, opts[0].impl_getname(), network,
-                              opts[1].impl_getname(), netmask, opts[2].impl_getname()))
+                msg = _('must be in network {0}/{1} ({2}/{3})')
+            return ValueError(msg.format(network, netmask,
+                              opts[1].impl_getname(), opts[2].impl_getname()))
         # test if ip is not network/broadcast IP
         return opts[2]._cons_ip_netmask((opts[2], opts[0]), (netmask, ip), warnings_only)
 
@@ -324,20 +324,19 @@ class PortOption(Option):
         if self._get_extra('_allow_range') and ":" in str(value):  # pragma: optional cover
             value = str(value).split(':')
             if len(value) != 2:
-                return ValueError(_('invalid port, range must have two values '
-                                  'only'))
+                return ValueError(_('range must have two values only'))
             if not value[0] < value[1]:
-                return ValueError(_('invalid port, first port in range must be'
+                return ValueError(_('first port in range must be'
                                   ' smaller than the second one'))
         else:
             value = [value]
 
         for val in value:
             if not self.port_re.search(val):
-                return ValueError(_('invalid port'))
+                return ValueError()
             val = int(val)
             if not self._get_extra('_min_value') <= val <= self._get_extra('_max_value'):  # pragma: optional cover
-                return ValueError(_('invalid port, must be an integer between {0} '
+                return ValueError(_('must be an integer between {0} '
                                     'and {1}').format(self._get_extra('_min_value'),
                                                       self._get_extra('_max_value')))
 
@@ -345,7 +344,7 @@ class PortOption(Option):
 class NetworkOption(Option):
     "represents the choice of a network"
     __slots__ = tuple()
-    display_name = _('network')
+    display_name = _('network address')
 
     def _validate(self, value, context=undefined, current_opt=undefined,
                   returns_raise=False):
@@ -353,29 +352,29 @@ class NetworkOption(Option):
         if err:
             return err
         if value.count('.') != 3:
-            return ValueError(_('invalid network address'))
+            return ValueError()
         for val in value.split('.'):
             if val.startswith("0") and len(val) > 1:
-                return ValueError(_('invalid network address'))
+                return ValueError()
         try:
             IP(value)
         except ValueError:  # pragma: optional cover
-            return ValueError(_('invalid network address'))
+            return ValueError()
 
     def _second_level_validation(self, value, warnings_only):
         ip = IP(value)
         if ip.iptype() == 'RESERVED':  # pragma: optional cover
             if warnings_only:
-                msg = _("network address is in reserved class")
+                msg = _("shouldn't be in reserved class")
             else:
-                msg = _("invalid network address, mustn't be in reserved class")
+                msg = _("mustn't be in reserved class")
             return ValueError(msg)
 
 
 class NetmaskOption(Option):
     "represents the choice of a netmask"
     __slots__ = tuple()
-    display_name = _('netmask')
+    display_name = _('netmask address')
 
     def _validate(self, value, context=undefined, current_opt=undefined,
                   returns_raise=False):
@@ -383,14 +382,14 @@ class NetmaskOption(Option):
         if err:
             return err
         if value.count('.') != 3:
-            return ValueError(_('invalid netmask address'))
+            return ValueError()
         for val in value.split('.'):
             if val.startswith("0") and len(val) > 1:
-                return ValueError(_('invalid netmask address'))
+                return ValueError()
         try:
             IP('0.0.0.0/{0}'.format(value))
         except ValueError:  # pragma: optional cover
-            return ValueError(_('invalid netmask address'))
+            return ValueError()
 
     def _cons_network_netmask(self, opts, vals, warnings_only):
         #opts must be (netmask, network) options
@@ -416,23 +415,20 @@ class NetmaskOption(Option):
             if make_net and ip.prefixlen() != 32:
                 val_ip = IP(val_ipnetwork)
                 if ip.net() == val_ip:
-                    msg = _("invalid IP {0} ({1}) with netmask {2},"
-                            " this IP is a network")
+                    msg = _("this is a network with netmask {0} ({1})")
                 if ip.broadcast() == val_ip:
-                    msg = _("invalid IP {0} ({1}) with netmask {2},"
-                            " this IP is a broadcast")
+                    msg = _("this is a broadcast with netmask {0} ({1})")
 
         except ValueError:  # pragma: optional cover
             if not make_net:
-                msg = _('invalid network {0} ({1}) with netmask {2}')
+                msg = _('with netmask {0} ({1})')
         if msg is not None:  # pragma: optional cover
-            return ValueError(msg.format(val_ipnetwork, opts[1].impl_getname(),
-                                         val_netmask))
+            return ValueError(msg.format(val_netmask, opts[1].impl_getname()))
 
 
 class BroadcastOption(Option):
     __slots__ = tuple()
-    display_name = _('broadcast')
+    display_name = _('broadcast address')
 
     def _validate(self, value, context=undefined, current_opt=undefined,
                   returns_raise=False):
@@ -442,7 +438,7 @@ class BroadcastOption(Option):
         try:
             IP('{0}/32'.format(value))
         except ValueError:  # pragma: optional cover
-            return ValueError(_('invalid broadcast address'))
+            return ValueError()
 
     def _cons_broadcast(self, opts, vals, warnings_only):
         if len(vals) != 3:
@@ -451,10 +447,8 @@ class BroadcastOption(Option):
             return
         broadcast, network, netmask = vals
         if IP('{0}/{1}'.format(network, netmask)).broadcast() != IP(broadcast):
-            return ValueError(_('invalid broadcast {0} ({1}) with network {2} '
-                                '({3}) and netmask {4} ({5})').format(
-                                    broadcast, opts[0].impl_getname(), network,
-                                    opts[1].impl_getname(), netmask, opts[2].impl_getname()))  # pragma: optional cover
+            return ValueError(_('with network {0}/{1} ({2}/{3})').format(
+                network, netmask, opts[1].impl_getname(), opts[2].impl_getname()))  # pragma: optional cover
 
 
 class DomainnameOption(Option):
@@ -504,9 +498,9 @@ class DomainnameOption(Option):
 
         def _valid_length(val):
             if len(val) < 1:
-                return ValueError(_("invalid domainname's length (min 1)"))
+                return ValueError(_("invalid length (min 1)"))
             if len(val) > part_name_length:
-                return ValueError(_("invalid domainname's length (max {0})"
+                return ValueError(_("invalid length (max {0})"
                                     "").format(part_name_length))
 
         if self._get_extra('_allow_ip') is True:  # pragma: optional cover
@@ -521,16 +515,16 @@ class DomainnameOption(Option):
             except ValueError:
                 pass
             else:
-                raise ValueError(_('invalid domainname, must not be an IP'))
+                raise ValueError(_('must not be an IP'))
         if self._get_extra('_dom_type') == 'netbios':
             part_name_length = 15
         else:
             part_name_length = 63
         if self._get_extra('_dom_type') == 'domainname':
             if not self._get_extra('_allow_without_dot') and not "." in value:
-                return ValueError(_("invalid domainname, must have dot"))
+                return ValueError(_("must have dot"))
             if len(value) > 255:
-                return ValueError(_("invalid domainname's length (max 255)"))
+                return ValueError(_("invalid length (max 255)"))
             for dom in value.split('.'):
                 err = _valid_length(dom)
                 if err:
@@ -546,7 +540,7 @@ class DomainnameOption(Option):
                 if warnings_only:
                     return ValueError(_('some characters may cause problems'))
                 else:
-                    return ValueError(_('invalid domainname'))
+                    return ValueError()
         #not for IP
         if self._get_extra('_allow_ip') is True:
             try:
@@ -564,7 +558,7 @@ class DomainnameOption(Option):
 class EmailOption(DomainnameOption):
     __slots__ = tuple()
     username_re = re.compile(r"^[\w!#$%&'*+\-/=?^`{|}~.]+$")
-    display_name = _('email')
+    display_name = _('email address')
 
     def _validate(self, value, context=undefined, current_opt=undefined,
                   returns_raise=False):
@@ -573,7 +567,7 @@ class EmailOption(DomainnameOption):
             return err
         splitted = value.split('@', 1)
         if len(splitted) == 1:
-            return ValueError(_('invalid email address, must contains one @'))
+            return ValueError(_('must contains one @'))
         username, domain = value.split('@', 1)
         if not self.username_re.search(username):
             return ValueError(_('invalid username in email address'))  # pragma: optional cover
@@ -599,7 +593,7 @@ class URLOption(DomainnameOption):
             return err
         match = self.proto_re.search(value)
         if not match:  # pragma: optional cover
-            return ValueError(_('invalid url, must start with http:// or '
+            return ValueError(_('must start with http:// or '
                                 'https://'))
         value = value[len(match.group(0)):]
         # get domain/files
@@ -617,7 +611,7 @@ class URLOption(DomainnameOption):
         else:
             domain, port = splitted
         if not 0 <= int(port) <= 65535:
-            return ValueError(_('invalid url, port must be an between 0 and '
+            return ValueError(_('port must be an between 0 and '
                                 '65536'))  # pragma: optional cover
         # validate domainname
         err = super(URLOption, self)._validate(domain)
@@ -628,7 +622,7 @@ class URLOption(DomainnameOption):
             return err
         # validate file
         if files is not None and files != '' and not self.path_re.search(files):
-            return ValueError(_('invalid url, must ends with a valid resource name'))  # pragma: optional cover
+            return ValueError(_('must ends with a valid resource name'))  # pragma: optional cover
 
     def _second_level_validation(self, value, warnings_only):
         pass
@@ -647,7 +641,7 @@ class UsernameOption(Option):
             return err
         match = self.username_re.search(value)
         if not match:
-            return ValueError(_('invalid username'))  # pragma: optional cover
+            return ValueError()  # pragma: optional cover
 
 
 class FilenameOption(Option):
@@ -662,4 +656,4 @@ class FilenameOption(Option):
             return err
         match = self.path_re.search(value)
         if not match:
-            return ValueError(_('invalid filename'))  # pragma: optional cover
+            return ValueError('some characters are not allowed')  # pragma: optional cover