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" <[email protected]> >> To: "Lynn Root" <[email protected]> >> Cc: [email protected] >> 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" <[email protected]> >>> To: "Jan Cholasta" <[email protected]> >>> Cc: "Lynn Root" <[email protected]>, [email protected] >>> 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 > _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
