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
From e84b355735f6594f8e51d2a4590adf0fb8e765b5 Mon Sep 17 00:00:00 2001 From: Lynn Root <lr...@redhat.com> Date: Mon, 29 Oct 2012 13:54:05 -0400 Subject: [PATCH] Switch %r specifiers to '%s' in Public errors for user-friendly readability 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 --- ipalib/backend.py | 6 +++--- ipalib/base.py | 12 ++++++------ ipalib/cli.py | 2 +- ipalib/config.py | 4 ++-- ipalib/constants.py | 10 +++++----- ipalib/errors.py | 4 ++-- ipalib/frontend.py | 8 ++++---- ipalib/output.py | 2 +- ipalib/rpc.py | 2 +- ipapython/ipautil.py | 2 +- tests/test_ipalib/test_base.py | 2 +- tests/test_ipalib/test_cli.py | 2 +- tests/test_ipalib/test_config.py | 2 +- tests/test_ipalib/test_frontend.py | 4 ++-- tests/test_ipalib/test_parameters.py | 4 ++-- 15 files changed, 33 insertions(+), 33 deletions(-) diff --git a/ipalib/backend.py b/ipalib/backend.py index 7be38ec..b245761 100644 --- a/ipalib/backend.py +++ b/ipalib/backend.py @@ -56,7 +56,7 @@ class Connectible(Backend): """ if hasattr(context, self.id): raise StandardError( - "connect: 'context.%s' already exists in thread %r" % ( + "connect: 'context.%s' already exists in thread '%s'" % ( self.id, threading.currentThread().getName() ) ) @@ -71,7 +71,7 @@ class Connectible(Backend): def disconnect(self): if not hasattr(context, self.id): raise StandardError( - "disconnect: 'context.%s' does not exist in thread %r" % ( + "disconnect: 'context.%s' does not exist in thread '%s'" % ( self.id, threading.currentThread().getName() ) ) @@ -93,7 +93,7 @@ class Connectible(Backend): Return thread-local connection. """ if not hasattr(context, self.id): - raise AttributeError('no context.%s in thread %r' % ( + raise AttributeError("no context.%s in thread '%s'" % ( self.id, threading.currentThread().getName()) ) return getattr(context, self.id).conn diff --git a/ipalib/base.py b/ipalib/base.py index 83d778f..17ca79d 100644 --- a/ipalib/base.py +++ b/ipalib/base.py @@ -170,9 +170,9 @@ def lock(instance): :param instance: The instance of `ReadOnly` (or similar) to lock. """ - assert instance.__islocked__() is False, 'already locked: %r' % instance + assert instance.__islocked__() is False, "already locked: '%s'" % instance instance.__lock__() - assert instance.__islocked__() is True, 'failed to lock: %r' % instance + assert instance.__islocked__() is True, "failed to lock: '%s'" % instance return instance @@ -198,7 +198,7 @@ def islocked(instance): """ assert ( hasattr(instance, '__lock__') and callable(instance.__lock__) - ), 'no __lock__() method: %r' % instance + ), 'no __lock__() method: %s' % instance return instance.__islocked__() @@ -224,7 +224,7 @@ def check_name(name): >>> check_name(u'my_name') Traceback (most recent call last): ... - TypeError: name: need a <type 'str'>; got u'my_name' (a <type 'unicode'>) + TypeError: name: need a '<type 'str'>; got u'my_name' (a '<type 'unicode'>')' So that `check_name()` can be easily used within an assignment, ``name`` is returned unchanged if it passes the check. For example: @@ -351,7 +351,7 @@ class NameSpace(ReadOnly): >>> ns.member3 = Member(3) # Lets add that missing 'member3' Traceback (most recent call last): ... - AttributeError: locked: cannot set NameSpace.member3 to Member(3) + AttributeError: locked: cannot set NameSpace.member3 to 'Member(3)' (For information on the locking protocol, see the `ReadOnly` class, of which `NameSpace` is a subclass.) @@ -406,7 +406,7 @@ class NameSpace(ReadOnly): raise AttributeError(OVERRIDE_ERROR % (self.__class__.__name__, name, self.__map[name], member) ) - assert not hasattr(self, name), 'Ouch! Has attribute %r' % name + assert not hasattr(self, name), 'Ouch! Has attribute %s' % name self.__map[name] = member setattr(self, name, member) lock(self) diff --git a/ipalib/cli.py b/ipalib/cli.py index ac0eb05..d937083 100644 --- a/ipalib/cli.py +++ b/ipalib/cli.py @@ -113,7 +113,7 @@ class textui(backend.Backend): """ if type(rows) not in (list, tuple): raise TypeError( - 'rows: need %r or %r; got %r' % (list, tuple, rows) + "rows: need '%s' or '%s'; got '%s'" % (list, tuple, rows) ) if len(rows) == 0: return 0 diff --git a/ipalib/config.py b/ipalib/config.py index 3c9aeaa..aa481ca 100644 --- a/ipalib/config.py +++ b/ipalib/config.py @@ -141,7 +141,7 @@ class Env(object): >>> env.date = 'Second' Traceback (most recent call last): ... - AttributeError: cannot override Env.date value u'First' with 'Second' + AttributeError: cannot override Env.date value 'First' with 'Second' An `Env` instance can be *locked*, after which no further variables can be set. Trying to set variables on a locked `Env` instance will also raise @@ -373,7 +373,7 @@ class Env(object): """ if path.abspath(config_file) != config_file: raise ValueError( - 'config_file must be an absolute path; got %r' % config_file + "config_file must be an absolute path; got '%s'" % config_file ) if not path.isfile(config_file): return diff --git a/ipalib/constants.py b/ipalib/constants.py index 81db020..e01321d 100644 --- a/ipalib/constants.py +++ b/ipalib/constants.py @@ -39,20 +39,20 @@ 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)' +TYPE_ERROR = "%s: need a '%s'; got '%s' (a '%s')'" # Stardard format for TypeError message when a callable is expected: -CALLABLE_ERROR = '%s: need a callable; got %r (which is a %r)' +CALLABLE_ERROR = "%s: need a callable; got '%s' (which is a '%s')" # Standard format for StandardError message when overriding an attribute: -OVERRIDE_ERROR = 'cannot override %s.%s value %r with %r' +OVERRIDE_ERROR = "cannot override %s.%s value '%s' with '%s'" # Standard format for AttributeError message when a read-only attribute is # already locked: -SET_ERROR = 'locked: cannot set %s.%s to %r' +SET_ERROR = "locked: cannot set %s.%s to '%s'" DEL_ERROR = 'locked: cannot delete %s.%s' # Used for a tab (or indentation level) when formatting for CLI: diff --git a/ipalib/errors.py b/ipalib/errors.py index a6c8e76..f7c24ec 100644 --- a/ipalib/errors.py +++ b/ipalib/errors.py @@ -254,7 +254,7 @@ class PublicError(StandardError): name = self.__class__.__name__ if self.format is not None and format is not None: raise ValueError( - 'non-generic %r needs format=None; got format=%r' % ( + "non-generic '%s' needs format=None; got format='%s'" % ( name, format) ) if message is None: @@ -290,7 +290,7 @@ class PublicError(StandardError): self.msg = message self.strerror = message for (key, value) in kw.iteritems(): - assert not hasattr(self, key), 'conflicting kwarg %s.%s = %r' % ( + assert not hasattr(self, key), 'conflicting kwarg %s.%s = %s' % ( name, key, value, ) setattr(self, key, value) diff --git a/ipalib/frontend.py b/ipalib/frontend.py index fadcb86..c614fe8 100644 --- a/ipalib/frontend.py +++ b/ipalib/frontend.py @@ -910,7 +910,7 @@ class Command(HasParam): """ nice = '%s.validate_output()' % self.name if not isinstance(output, dict): - raise TypeError('%s: need a %r; got a %r: %r' % ( + raise TypeError("%s: need a '%s'; got a '%s': '%s'" % ( nice, dict, type(output), output) ) expected_set = set(self.output) @@ -918,18 +918,18 @@ class Command(HasParam): if expected_set != actual_set: missing = expected_set - actual_set if missing: - raise ValueError('%s: missing keys %r in %r' % ( + raise ValueError('%s: missing keys %s in %s' % ( nice, sorted(missing), output) ) extra = actual_set - expected_set if extra: - raise ValueError('%s: unexpected keys %r in %r' % ( + raise ValueError('%s: unexpected keys %s in %s' % ( nice, sorted(extra), output) ) for o in self.output(): value = output[o.name] if not (o.type is None or isinstance(value, o.type)): - raise TypeError('%s:\n output[%r]: need %r; got %r: %r' % ( + raise TypeError('%s:\n output[%s]: need %s; got %s: %s' % ( nice, o.name, o.type, type(value), value) ) if callable(o.validate): diff --git a/ipalib/output.py b/ipalib/output.py index 1f42b4d..437040b 100644 --- a/ipalib/output.py +++ b/ipalib/output.py @@ -93,7 +93,7 @@ class Entry(Output): emsg = """%s.validate_output() => %s.validate(): - output[%r][%d]: need a %r; got a %r: %r""" + output['%s'][%d]: need a '%s'; got a '%s': '%s'""" class ListOfEntries(Output): type = (list, tuple) diff --git a/ipalib/rpc.py b/ipalib/rpc.py index c555105..89cc0f3 100644 --- a/ipalib/rpc.py +++ b/ipalib/rpc.py @@ -527,7 +527,7 @@ class xmlclient(Connectible): """ if name not in self.Command: raise ValueError( - '%s.forward(): %r not in api.Command' % (self.name, name) + "%s.forward(): '%s' not in api.Command" % (self.name, name) ) server = self.reconstruct_url() self.info('Forwarding %r to server %r', name, server) diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py index e76d87d..791f380 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 diff --git a/tests/test_ipalib/test_base.py b/tests/test_ipalib/test_base.py index 78c8be0..0c3365b 100644 --- a/tests/test_ipalib/test_base.py +++ b/tests/test_ipalib/test_base.py @@ -96,7 +96,7 @@ def test_lock(): assert f(o) is o assert o.__islocked__() is True e = raises(AssertionError, f, o) - assert str(e) == 'already locked: %r' % o + assert str(e) == "already locked: '%s'" % o # Test with another class implemented locking protocol: class Lockable(object): diff --git a/tests/test_ipalib/test_cli.py b/tests/test_ipalib/test_cli.py index 0a6eeee..f205a7f 100644 --- a/tests/test_ipalib/test_cli.py +++ b/tests/test_ipalib/test_cli.py @@ -34,7 +34,7 @@ class test_textui(ClassChecker): """ o = self.cls() e = raises(TypeError, o.max_col_width, 'hello') - assert str(e) == 'rows: need %r or %r; got %r' % (list, tuple, 'hello') + assert str(e) == "rows: need '%s' or '%s'; got '%s'" % (list, tuple, 'hello') rows = [ 'hello', 'naughty', diff --git a/tests/test_ipalib/test_config.py b/tests/test_ipalib/test_config.py index e729a62..22438c5 100644 --- a/tests/test_ipalib/test_config.py +++ b/tests/test_ipalib/test_config.py @@ -175,7 +175,7 @@ class test_Env(ClassChecker): assert base.lock(o) is o assert o.__islocked__() is True e = raises(AssertionError, base.lock, o) - assert str(e) == 'already locked: %r' % o + assert str(e) == "already locked: '%s'" % o def test_islocked(self): """ diff --git a/tests/test_ipalib/test_frontend.py b/tests/test_ipalib/test_frontend.py index 528609d..2f0fa57 100644 --- a/tests/test_ipalib/test_frontend.py +++ b/tests/test_ipalib/test_frontend.py @@ -628,7 +628,7 @@ class test_Command(ClassChecker): # Test with wrong type: wrong = ('foo', 'bar', 'baz') e = raises(TypeError, inst.validate_output, wrong) - assert str(e) == '%s.validate_output(): need a %r; got a %r: %r' % ( + assert str(e) == "%s.validate_output(): need a '%s'; got a '%s': '%s'" % ( 'Example', dict, tuple, wrong ) @@ -668,7 +668,7 @@ class test_Command(ClassChecker): wrong = dict(foo=17.9, bar=[18]) e = raises(TypeError, inst.validate_output, wrong) - assert str(e) == '%s:\n output[%r]: need %r; got %r: %r' % ( + assert str(e) == "%s:\n output['%s']: need '%s'; got '%s': '%s'" % ( 'Complex.validate_output()', 'foo', int, float, 17.9 ) diff --git a/tests/test_ipalib/test_parameters.py b/tests/test_ipalib/test_parameters.py index e6ac91d..9ef842e 100644 --- a/tests/test_ipalib/test_parameters.py +++ b/tests/test_ipalib/test_parameters.py @@ -470,7 +470,7 @@ class test_Param(ClassChecker): # 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)) + assert str(e) == "invalid '%s'" % (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)) @@ -536,7 +536,7 @@ class test_Param(ClassChecker): # 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)) + 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, True, index=3.0) -- 1.7.12
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
