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
