Re: [Freeipa-devel] [PATCH] 317 Improve StrEnum validation error message

2012-10-01 Thread Martin Kosek 
On 10/01/2012 11:16 AM, Jan Cholasta wrote:
> Dne 1.10.2012 10:05, Martin Kosek napsal(a):
>> 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 , as we 
>>> use
>>> %r in more places where we should not.
>>>
>>> Honza
>>>
>>
>> Thanks. New patch attached.
>>
>> Martin
>>
> 
> ACK.
> 
> Honza
> 

Pushed to master, ipa-3-0.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 317 Improve StrEnum validation error message

2012-10-01 Thread Jan Cholasta 

Dne 1.10.2012 10:05, Martin Kosek napsal(a):

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 , as we use
%r in more places where we should not.

Honza



Thanks. New patch attached.

Martin



ACK.

Honza

--
Jan Cholasta

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 317 Improve StrEnum validation error message

2012-10-01 Thread Martin Kosek 
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 , 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 
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(du

Re: [Freeipa-devel] [PATCH] 317 Improve StrEnum validation error message

2012-10-01 Thread Jan Cholasta 

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__.


Also, I have opened , as 
we use %r in more places where we should not.


Honza

--
Jan Cholasta

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

[Freeipa-devel] [PATCH] 317 Improve StrEnum validation error message

2012-09-27 Thread Martin Kosek 
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
From ea2e782055d4c305475cbf5ca2029823f1e13a57 Mon Sep 17 00:00:00 2001
From: Martin Kosek 
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 | 13 -
 tests/test_ipalib/test_parameters.py | 19 +--
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index 53756a80a422135e99a3ecd1e9511e037e52c0dc..770e049108a493f98eecd02ca34715afe60a3977 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -1597,10 +1597,13 @@ class Enum(Param):
 
 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:
+values = u', '.join("'%s'" % value for value in self.values)
+return _('must be one of %(values)s') % dict(values=values)
+elif len(self.values) == 1:
+return _("must be '%(value)s'") % dict(value=self.values[0])
+else:
+return _("must be empty")
 
 class BytesEnum(Enum):
 """
@@ -1622,7 +1625,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..be7a1d985ca078e877423493e10f694ffd6c1ea9 100644
--- a/tests/test_ipalib/test_parameters.py
+++ b/tests/test_ipalib/test_parameters.py
@@ -1147,7 +1147,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 +1161,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