Hmm I hope I understand well enough this time around.

However, when I run the tests, there's this one error message I come across 
from `test_user[97]: user_add: Create u'tuser2'` - it throws `DatabaseError: 
Type or value exists`.  I'm a bit lost on how to track this down.

Once again - thanks for your help!

Lynn Root
Associate Software Engineer
Red Hat

----- Original Message -----
From: "Martin Kosek" <mko...@redhat.com>
To: "Jan Cholasta" <jchol...@redhat.com>
Cc: "Lynn Root" <lr...@redhat.com>, freeipa-devel@redhat.com
Sent: Thursday, November 8, 2012 8:46:42 AM
Subject: Re: [Freeipa-devel] [PATCH] Switch %r specifiers to %s in Public errors

On 11/07/2012 06:46 PM, Jan Cholasta wrote:
> On 7.11.2012 16:08, Lynn Root wrote:
>> Third time is a charm?
>>
>> Lynn Root
>> Associate Software Engineer
>> Red Hat
>>
>> ----- Original Message -----
>> From: "Jan Cholasta" <jchol...@redhat.com>
>> To: "Lynn Root" <lr...@redhat.com>
>> Cc: freeipa-devel@redhat.com
>> Sent: Monday, November 5, 2012 10:25:32 AM
>> Subject: Re: [Freeipa-devel] [PATCH] Switch %r specifiers to %s in Public 
>> errors
>>
>> On 5.11.2012 09:43, Lynn Root wrote:
>>> Here's try #2! Adjusted patch attached.  Let me know if there's anything
>>> else I've missed.
>>>
>>> Switched %r specifiers to '%s' in Public errors, and adjusted tests to
>>> expect no preceding 'u'.
>>>
>>> Tickets: https://fedorahosted.org/freeipa/ticket/3121 &
>>> https://fedorahosted.org/freeipa/ticket/2588
>>>
>>> Lynn Root
>>> Associate Software Engineer
>>> Red Hat
>>>
>>> ----- Original Message -----
>>> From: "Martin Kosek" <mko...@redhat.com>
>>> To: "Jan Cholasta" <jchol...@redhat.com>
>>> Cc: "Lynn Root" <lr...@redhat.com>, freeipa-devel@redhat.com
>>> Sent: Tuesday, October 30, 2012 9:08:33 AM
>>> Subject: Re: [Freeipa-devel] [PATCH] Switch %r specifiers to %s in Public
>>> errors
>>>
>>> On 10/30/2012 09:04 AM, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> On 29.10.2012 19:54, Lynn Root wrote:
>>>>> Hi all!
>>>>>
>>>>> This switch drops the preceding 'u' from strings in public error messages.
>>>>>
>>>>> Ticket: https://fedorahosted.org/freeipa/ticket/3121
>>>>>
>>>>> This patch also addresses the unfriendly 'u' from re-raising errors from 
>>>>> the
>>>>> external call to netaddr.IPAddress by passing a bytestring to the 
>>>>> function.
>>>>>
>>>>> Ticket: https://fedorahosted.org/freeipa/ticket/2588
>>>>>
>>>>>
>>>>> My first patch (and freeipa dev list email) ever! Let me know where 
>>>>> there's
>>>>> room to improve.
>>>>>
>>>>> Lynn Root
>>>>> Associate Software Engineer
>>>>> Red Hat
>>>>>
>>>>
>>>> I think it would be nice if you kept the quotes around the values, as that 
>>>> is
>>>> probably the reason "%r" was used in the first place - i.e. use "'%s'" 
>>>> instead
>>>> of plain "%s".
>>>
>>> +1
>>>
>>> With current patch, I assume that a lot of unit tests will fail as they 
>>> check
>>> exact error message wording. I'd recommend running the whole test suite with
>>> your second patch revision. There is a short walkthrough how to set it up:
>>>
>>> http://freeipa.org/page/Testing
>>>
>>> Martin
>>>
>>
>> You missed a few:
>>
>> $ git grep -En '%(\(.*?\))?r'
>>
>> Honza
>>
> 
> I think you have gone too far this time :-) It is not necessary (or wise) to
> get rid of %r *everywhere* in the code.

Thanks Honza for pointing that out. It seems I missed that in yesterday's
review. Now, when I look at it, it indeed is not right.

> 
> A few rules to keep in mind:
> 
>   * If it is not an error message, do not touch it (log messages are not error
> messages BTW).
> 
>   * If it is an error message for an exception that does not inherit from
> errors.PublicError, do not touch it (there might be a few exceptions, though).

Right. But for example, your netaddr str conversions should be fine since the
netaddr error is propagated up to the ValidationError.

Martin

> 
>   * Use '%s' (%s with ticks) only for arguments whose value can be only str or
> unicode.
> 
> Honza
> 

From 7ad684c977f047dc1f1c6995fc779695760a4a8a Mon Sep 17 00:00:00 2001
From: Lynn Root <lr...@redhat.com>
Date: Thu, 8 Nov 2012 10:06:35 -0500
Subject: [PATCH] Switch %r specifiers to '%s' in Public errors

This switch drops the preceding 'u' from strings within Public error messages.

Ticket: https://fedorahosted.org/freeipa/ticket/3121

This patch also addresses the unfriendly 'u' from re-raising errors from netaddr.IPAddress by passing a bytestring through the function.
---
 ipalib/constants.py                  |  2 +-
 ipalib/errors.py                     | 38 ++++++++++++++++++------------------
 ipalib/plugins/dns.py                |  6 +++---
 ipalib/util.py                       |  2 +-
 ipapython/ipautil.py                 |  6 +++---
 tests/test_xmlrpc/test_dns_plugin.py |  4 ++--
 6 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/ipalib/constants.py b/ipalib/constants.py
index 81db020..7970994 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -39,7 +39,7 @@ NULLS = (None, '', u'', tuple(), [])
 NAME_REGEX = r'^[a-z][_a-z0-9]*[a-z0-9]$|^[a-z]$'
 
 # Format for ValueError raised when name does not match above regex:
-NAME_ERROR = 'name must match %r; got %r'
+NAME_ERROR = "name must match '%s'; got '%s'"
 
 # Standard format for TypeError message:
 TYPE_ERROR = '%s: need a %r; got %r (a %r)'
diff --git a/ipalib/errors.py b/ipalib/errors.py
index a6c8e76..ba0505d 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -166,7 +166,7 @@ class PluginSubclassError(PrivateError):
 
     """
 
-    format  = '%(plugin)r not subclass of any base in %(bases)r'
+    format = '%(plugin)r not subclass of any base in %(bases)r'
 
 
 class PluginDuplicateError(PrivateError):
@@ -311,7 +311,7 @@ class VersionError(PublicError):
     """
 
     errno = 901
-    format = _('%(cver)s client incompatible with %(sver)s server at %(server)r')
+    format = _("%(cver)s client incompatible with %(sver)s server at '%(server)s'")
 
 
 class UnknownError(PublicError):
@@ -367,7 +367,7 @@ class ServerInternalError(PublicError):
     """
 
     errno = 904
-    format = _('an internal error has occurred on server at %(server)r')
+    format = _("an internal error has occurred on server at '%(server)s'")
 
 
 class CommandError(PublicError):
@@ -383,7 +383,7 @@ class CommandError(PublicError):
     """
 
     errno = 905
-    format = _('unknown command %(name)r')
+    format = _("unknown command '%(name)s'")
 
 
 class ServerCommandError(PublicError):
@@ -400,7 +400,7 @@ class ServerCommandError(PublicError):
     """
 
     errno = 906
-    format = _('error on server %(server)r: %(error)s')
+    format = _("error on server '%(server)s': %(error)s")
 
 
 class NetworkError(PublicError):
@@ -416,7 +416,7 @@ class NetworkError(PublicError):
     """
 
     errno = 907
-    format = _('cannot connect to %(uri)r: %(error)s')
+    format = _("cannot connect to '%(uri)s': %(error)s")
 
 
 class ServerNetworkError(PublicError):
@@ -425,7 +425,7 @@ class ServerNetworkError(PublicError):
     """
 
     errno = 908
-    format = _('error on server %(server)r: %(error)s')
+    format = _("error on server '%(server)s': %(error)s")
 
 
 class JSONError(PublicError):
@@ -526,7 +526,7 @@ class ServiceError(KerberosError):
     """
 
     errno = 1102
-    format = _('Service %(service)r not found in Kerberos database')
+    format = _("Service '%(service)s' not found in Kerberos database")
 
 
 class NoCCacheError(KerberosError):
@@ -688,7 +688,7 @@ class ZeroArgumentError(InvocationError):
     """
 
     errno = 3003
-    format = _('command %(name)r takes no arguments')
+    format = _("command '%(name)s' takes no arguments")
 
 
 class MaxArgumentError(InvocationError):
@@ -708,8 +708,8 @@ class MaxArgumentError(InvocationError):
     def __init__(self, message=None, **kw):
         if message is None:
             format = ungettext(
-                'command %(name)r takes at most %(count)d argument',
-                'command %(name)r takes at most %(count)d arguments',
+                "command '%(name)s' takes at most %(count)d argument",
+                "command '%(name)s' takes at most %(count)d arguments",
                 kw['count']
             )
         else:
@@ -734,11 +734,11 @@ class OverlapError(InvocationError):
     >>> raise OverlapError(names=['givenname', 'login'])
     Traceback (most recent call last):
       ...
-    OverlapError: overlapping arguments and options: ['givenname', 'login']
+    OverlapError: overlapping arguments and options: '['givenname', 'login']'
     """
 
     errno = 3006
-    format = _('overlapping arguments and options: %(names)r')
+    format = _("overlapping arguments and options: '%(names)s'")
 
 
 class RequirementError(InvocationError):
@@ -754,7 +754,7 @@ class RequirementError(InvocationError):
     """
 
     errno = 3007
-    format = _('%(name)r is required')
+    format = _("'%(name)s' is required")
 
 
 class ConversionError(InvocationError):
@@ -770,7 +770,7 @@ class ConversionError(InvocationError):
     """
 
     errno = 3008
-    format = _('invalid %(name)r: %(error)s')
+    format = _("invalid '%(name)s': %(error)s")
 
 
 class ValidationError(InvocationError):
@@ -786,7 +786,7 @@ class ValidationError(InvocationError):
     """
 
     errno = 3009
-    format = _('invalid %(name)r: %(error)s')
+    format = _("invalid '%(name)s': %(error)s")
 
 
 class NoSuchNamespaceError(InvocationError):
@@ -802,7 +802,7 @@ class NoSuchNamespaceError(InvocationError):
     """
 
     errno = 3010
-    format = _('api has no such namespace: %(name)r')
+    format = _("api has no such namespace: '%(name)s'")
 
 
 class PasswordMismatch(InvocationError):
@@ -978,7 +978,7 @@ class MalformedUserPrincipal(ExecutionError):
     """
 
     errno = 4008
-    format = _('Principal is not of the form user@REALM: %(principal)r')
+    format = _("Principal is not of the form user@REALM: '%(principal)s'")
 
 class AlreadyActive(ExecutionError):
     """
@@ -1357,7 +1357,7 @@ class HelpError(BuiltinError):
     """
 
     errno = 4101
-    format = _('no command nor help topic %(topic)r')
+    format = _("no command nor help topic '%(topic)s'")
 
 
 class LDAPError(ExecutionError):
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index febd4d1..626e27d 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -256,7 +256,7 @@ def _create_zone_serial():
 
 def _reverse_zone_name(netstr):
     try:
-        netaddr.IPAddress(netstr)
+        netaddr.IPAddress(str(netstr))
     except (netaddr.AddrFormatError, ValueError):
         pass
     else:
@@ -274,7 +274,7 @@ def _reverse_zone_name(netstr):
 
 def _validate_ipaddr(ugettext, ipaddr, ip_version=None):
     try:
-        ip = netaddr.IPAddress(ipaddr, flags=netaddr.INET_PTON)
+        ip = netaddr.IPAddress(str(ipaddr), flags=netaddr.INET_PTON)
 
         if ip_version is not None:
             if ip.version != ip_version:
@@ -441,7 +441,7 @@ def add_forward_record(zone, name, str_address):
         pass # the entry already exists and matches
 
 def get_reverse_zone(ipaddr, prefixlen=None):
-    ip = netaddr.IPAddress(ipaddr)
+    ip = netaddr.IPAddress(str(ipaddr))
     revdns = unicode(ip.reverse_dns)
 
     if prefixlen is None:
diff --git a/ipalib/util.py b/ipalib/util.py
index 3fe5c9f..16441a3 100644
--- a/ipalib/util.py
+++ b/ipalib/util.py
@@ -511,7 +511,7 @@ def zone_is_reverse(zone_name):
     return False
 
 def get_reverse_zone_default(ip_address):
-    ip = netaddr.IPAddress(ip_address)
+    ip = netaddr.IPAddress(str(ip_address))
     items = ip.reverse_dns.split('.')
 
     if ip.version == 4:
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index e76d87d..92f2522 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -103,7 +103,7 @@ class CheckedIPAddress(netaddr.IPAddress):
         else:
             try:
                 try:
-                    addr = netaddr.IPAddress(addr, flags=self.netaddr_ip_flags)
+                    addr = netaddr.IPAddress(str(addr), flags=self.netaddr_ip_flags)
                 except netaddr.AddrFormatError:
                     # netaddr.IPAddress doesn't handle zone indices in textual
                     # IPv6 addresses. Try removing zone index and parse the
@@ -113,11 +113,11 @@ class CheckedIPAddress(netaddr.IPAddress):
                     addr, sep, foo = addr.partition('%')
                     if sep != '%':
                         raise
-                    addr = netaddr.IPAddress(addr, flags=self.netaddr_ip_flags)
+                    addr = netaddr.IPAddress(str(addr), flags=self.netaddr_ip_flags)
                     if addr.version != 6:
                         raise
             except ValueError:
-                net = netaddr.IPNetwork(addr, flags=self.netaddr_ip_flags)
+                net = netaddr.IPNetwork(str(addr), flags=self.netaddr_ip_flags)
                 if not parse_netmask:
                     raise ValueError("netmask and prefix length not allowed here")
                 addr = net.ip
diff --git a/tests/test_xmlrpc/test_dns_plugin.py b/tests/test_xmlrpc/test_dns_plugin.py
index 3c2dc00..b5c33b0 100644
--- a/tests/test_xmlrpc/test_dns_plugin.py
+++ b/tests/test_xmlrpc/test_dns_plugin.py
@@ -1086,7 +1086,7 @@ class test_dns(Declarative):
             desc='Try to add invalid allow-query to zone %r' % dnszone1,
             command=('dnszone_mod', [dnszone1], {'idnsallowquery': u'foo'}),
             expected=errors.ValidationError(name='allow_query',
-                error=u"failed to detect a valid IP address from u'foo'"),
+                error=u"failed to detect a valid IP address from 'foo'"),
         ),
 
         dict(
@@ -1119,7 +1119,7 @@ class test_dns(Declarative):
             desc='Try to add invalid allow-transfer to zone %r' % dnszone1,
             command=('dnszone_mod', [dnszone1], {'idnsallowtransfer': u'10.'}),
             expected=errors.ValidationError(name='allow_transfer',
-                error=u"failed to detect a valid IP address from u'10.'"),
+                error=u"failed to detect a valid IP address from '10.'"),
         ),
 
         dict(
-- 
1.7.12

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

Reply via email to