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: Friday, November 9, 2012 3:25:20 PM > Subject: Re: [Freeipa-devel] [PATCH] Switch %r specifiers to %s in Public > errors > > On 8.11.2012 17:22, Lynn Root wrote: > > 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 > >> > > > > This is better, thanks. > > In OverlapError.format, remove the ticks around %s, as we expect a > list > here (I think we could make it look prettier, similar to what Martin > did > in > <https://fedorahosted.org/freeipa/changeset/988ea368272822f2153563ad34554240e3377d60/>, > but I'm not sure if we want to do it in this ticket/patch). > Fixed, ty. > I'm not sure what to do about the ValidationError at > ipalib/parameters.py:882 and ipalib/parameters.py:1171. I think it > should be "TypeError(TYPE_ERROR % (self.name, self.type, value, > type(value)))" instead, as by the time parameters are validated they > are > the right type. Done - with adjusted tests. > > Also there is one %r you missed in ipalib/parameters.py:1554. The tests seem to be expecting a unicode character - are you sure this is right? If not - attached is the new patch (correctly formatted). Thanks again! > > Honza > > -- > Jan Cholasta >
From 1dba5dd539455e47f978e3822e32a3ed82ea8726 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. This patch also addresses the related unfriendly 'u' from re-raising errors from netaddr.IPAddress by passing a bytestring through the function. Also switched ValidationError to TypeError in validate_scalar per Honza Cholasta. Ticket: https://fedorahosted.org/freeipa/ticket/3121 Ticket: https://fedorahosted.org/freeipa/ticket/2588 --- ipalib/constants.py | 2 +- ipalib/errors.py | 36 ++++++++++++++++++------------------ ipalib/parameters.py | 14 +++++--------- ipalib/plugins/dns.py | 6 +++--- ipalib/util.py | 2 +- ipapython/ipautil.py | 6 +++--- tests/test_ipalib/test_parameters.py | 16 ++++++++-------- tests/test_xmlrpc/test_dns_plugin.py | 4 ++-- 8 files changed, 41 insertions(+), 45 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..a391ada 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: @@ -738,7 +738,7 @@ class OverlapError(InvocationError): """ 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/parameters.py b/ipalib/parameters.py index b3a75f2..edead34 100644 --- a/ipalib/parameters.py +++ b/ipalib/parameters.py @@ -879,10 +879,8 @@ class Param(ReadOnly): def _validate_scalar(self, value, index=None): if type(value) is not self.type: - raise ValidationError(name=self.name, - error='need a %r; got %r (a %r)' % ( - self.type, value, type(value) - ) + raise TypeError( + TYPE_ERROR % (self.name, self.type, value, type(value)) ) if index is not None and type(index) is not int: raise TypeError( @@ -1167,11 +1165,9 @@ class Int(Number): the exception that it allows both int and long types. The min/max rules handle size enforcement. """ - if type(value) not in (int, long): - raise ValidationError(name=self.name, - error='need a %r; got %r (a %r)' % ( - self.type, value, type(value) - ) + if type(value) not in (int, long): + raise TypeError( + TYPE_ERROR % (self.name, self.type, value, type(value)) ) if index is not None and type(index) is not int: raise TypeError( 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_ipalib/test_parameters.py b/tests/test_ipalib/test_parameters.py index e6ac91d..e94bd26 100644 --- a/tests/test_ipalib/test_parameters.py +++ b/tests/test_ipalib/test_parameters.py @@ -469,11 +469,11 @@ class test_Param(ClassChecker): assert str(e) == 'value: empty tuple must be converted to None' # Test with wrong (scalar) type: - e = raises(ValidationError, o.validate, (None, None, 42, None), 'cli') - assert str(e) == 'invalid %s' % (TYPE_ERROR % ('\'my_param\'', NoneType, 42, int)) + e = raises(TypeError, o.validate, (None, None, 42, None), 'cli') + assert str(e) == TYPE_ERROR % ('my_param', NoneType, 42, int) o = self.cls('my_param') - e = raises(ValidationError, o.validate, 'Hello', 'cli') - assert str(e) == 'invalid %s' % (TYPE_ERROR % ('\'my_param\'', NoneType, 'Hello', str)) + e = raises(TypeError, o.validate, 'Hello', 'cli') + assert str(e) == TYPE_ERROR % ('my_param', NoneType, 'Hello', str) class Example(self.cls): type = int @@ -535,10 +535,10 @@ class test_Param(ClassChecker): o = MyParam('my_param', okay) # Test that TypeError is appropriately raised: - e = raises(ValidationError, o._validate_scalar, 0) - assert str(e) == 'invalid %s' % (TYPE_ERROR % ('\'my_param\'', bool, 0, int)) - e = raises(ValidationError, o._validate_scalar, 'Hi', index=4) - assert str(e) == 'invalid %s' % (TYPE_ERROR % ('\'my_param\'', bool, 'Hi', str)) + e = raises(TypeError, o._validate_scalar, 0) + assert str(e) == TYPE_ERROR % ('my_param', bool, 0, int) + e = raises(TypeError, o._validate_scalar, 'Hi', index=4) + assert str(e) == TYPE_ERROR % ('my_param', bool, 'Hi', str) e = raises(TypeError, o._validate_scalar, True, index=3.0) assert str(e) == TYPE_ERROR % ('index', int, 3.0, float) 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
