[Freeipa-devel] [PATCH] Switch %r specifiers to %s in Public errors

[Martin Kosek]

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" <jcholast at redhat.com>
>> To: "Lynn Root" <lroot at redhat.com>
>> Cc: freeipa-devel at 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" <mkosek at redhat.com>
>>> To: "Jan Cholasta" <jcholast at redhat.com>
>>> Cc: "Lynn Root" <lroot at redhat.com>, freeipa-devel at 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
>