----- Original Message ----- > On 12.11.2012 12:50, Lynn Root wrote: > > > > > > 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. > > Thanks, but please refer to me as jchol...@redhat.com in the commit > message, so that people don't have to look me up. >
Fixed. > > > >> > >> 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? > > Currently the message the error produces has two ticks on each side > of > the value, which is ugly. So, replace the "\'%(char)r\'" with either > "\'%(char)s\'" or "%(char)r". Ah now I see - fixed. > > > > > If not - attached is the new patch (correctly formatted). > > > > Thanks again! > > > >> > >> Honza > >> > >> -- > >> Jan Cholasta > >> > > Honza > > -- > Jan Cholasta > Lynn Root Associate Software Engineer Red Hat
From 0a36085bf8978d078561bef27e6382ae1a4c8fa7 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 jchola...@redhat.com. 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 | 16 ++++++---------- ipalib/plugins/dns.py | 6 +++--- ipalib/util.py | 2 +- ipapython/ipautil.py | 6 +++--- tests/test_ipalib/test_parameters.py | 18 +++++++++--------- tests/test_xmlrpc/test_dns_plugin.py | 4 ++-- 8 files changed, 43 insertions(+), 47 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..670e036 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( @@ -1553,7 +1549,7 @@ class IA5Str(Str): if ord(value[i]) > 127: raise ConversionError(name=self.get_param_name(), index=index, - error=_('The character \'%(char)r\' is not allowed.') % + error=_('The character %(char)r is not allowed.') % dict(char=value[i],) ) return super(IA5Str, self)._convert_scalar(value, index) 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..b30ae5a 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) @@ -1574,4 +1574,4 @@ class test_IA5Str(ClassChecker): e = raises(errors.ConversionError, mthd, value) assert e.name == 'my_str' assert e.index is None - assert_equal(e.error, "The character \''\\xc3'\' is not allowed.") + assert_equal(e.error, "The character '\\xc3' is not allowed.") 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
