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.

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).

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

Honza

--
Jan Cholasta

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

Reply via email to