On 10/01/2012 09:19 AM, Jan Cholasta wrote: > Dne 27.9.2012 14:28, Martin Kosek napsal(a): >> Do not print list of possible values as "%r" but simply as a list >> of quoted values which should make it easier to read for users. >> Also add a special case when there is just one allowed value. >> >> https://fedorahosted.org/freeipa/ticket/2869 >> >> >> Examples of the improved Enum validation error messages: >> >> # ipa automember-add foo --type=bar >> ipa: ERROR: invalid 'type': must be one of 'group', 'hostgroup' >> >> # ipa trust-add foo --type=foo >> ipa: ERROR: invalid 'type': must be 'ad' >> >> Martin >> > > IMO instead of doing this: > > + else: > + return _("must be empty") > > we should not allow empty "values" kwarg in Enum at all, i.e. check that > len(self.values) > 0 in Enum.__init__.
Right, I fixed it. I also added a relevant test case to our unit tests. > > Also, I have opened <https://fedorahosted.org/freeipa/ticket/3121>, as we use > %r in more places where we should not. > > Honza > Thanks. New patch attached. Martin
From ba830b681b95b347675031e27fff5cde8a9242fb Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Thu, 27 Sep 2012 14:18:02 +0200 Subject: [PATCH] Improve StrEnum validation error message Do not print list of possible values as "%r" but simply as a list of quoted values which should make it easier to read for users. Also add a special case when there is just one allowed value. https://fedorahosted.org/freeipa/ticket/2869 --- ipalib/parameters.py | 15 ++++++++++----- tests/test_ipalib/test_parameters.py | 25 +++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/ipalib/parameters.py b/ipalib/parameters.py index 53756a80a422135e99a3ecd1e9511e037e52c0dc..b3a75f288f895449cfa460c4c1512853248c8cd9 100644 --- a/ipalib/parameters.py +++ b/ipalib/parameters.py @@ -1595,12 +1595,17 @@ class Enum(Param): TYPE_ERROR % (n, self.type, v, type(v)) ) + if len(self.values) < 1: + raise ValueError( + '%s: list of values must not be empty' % self.nice) + def _rule_values(self, _, value, **kw): if value not in self.values: - return _('must be one of %(values)r') % dict( - values=self.values, - ) - + if len(self.values) == 1: + return _("must be '%(value)s'") % dict(value=self.values[0]) + else: + values = u', '.join("'%s'" % value for value in self.values) + return _('must be one of %(values)s') % dict(values=values) class BytesEnum(Enum): """ @@ -1622,7 +1627,7 @@ class StrEnum(Enum): >>> enum.validate(u'Four', 'cli') Traceback (most recent call last): ... - ValidationError: invalid 'my_enum': must be one of (u'One', u'Two', u'Three') + ValidationError: invalid 'my_enum': must be one of 'One', 'Two', 'Three' """ type = unicode diff --git a/tests/test_ipalib/test_parameters.py b/tests/test_ipalib/test_parameters.py index 0b6fae375639ee0e012a9cee12311adc62b63934..e6ac91db787c4b494f525641dd1aab989eb55ef0 100644 --- a/tests/test_ipalib/test_parameters.py +++ b/tests/test_ipalib/test_parameters.py @@ -1140,6 +1140,12 @@ class test_StrEnum(ClassChecker): "StrEnum('my_enum') values[1]", unicode, 'naughty', str ) + # Test that ValueError is raised when list of values is empty + badvalues = tuple() + e = raises(ValueError, self.cls, 'empty_enum', values=badvalues) + assert_equal(str(e), "StrEnum('empty_enum'): list of values must not " + "be empty") + def test_rules_values(self): """ Test the `ipalib.parameters.StrEnum._rule_values` method. @@ -1147,7 +1153,7 @@ class test_StrEnum(ClassChecker): values = (u'Hello', u'naughty', u'nurse!') o = self.cls('my_enum', values=values) rule = o._rule_values - translation = u'values=%(values)s' + translation = u"values='Hello', 'naughty', 'nurse!'" dummy = dummy_ugettext(translation) # Test with passing values: @@ -1161,7 +1167,22 @@ class test_StrEnum(ClassChecker): rule(dummy, val), translation % dict(values=values), ) - assert_equal(dummy.message, 'must be one of %(values)r') + assert_equal(dummy.message, "must be one of %(values)s") + dummy.reset() + + # test a special case when we have just one allowed value + values = (u'Hello', ) + o = self.cls('my_enum', values=values) + rule = o._rule_values + translation = u"value='Hello'" + dummy = dummy_ugettext(translation) + + for val in (u'Howdy', u'quiet', u'library!'): + assert_equal( + rule(dummy, val), + translation % dict(values=values), + ) + assert_equal(dummy.message, "must be '%(value)s'") dummy.reset() -- 1.7.11.4
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
