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

[Jan Cholasta]

On 12.11.2012 12:50, Lynn Root wrote:
>
>
> 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: 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" <mkosek at redhat.com>
>>> To: "Jan Cholasta" <jcholast at redhat.com>
>>> Cc: "Lynn Root" <lroot at redhat.com>, freeipa-devel at 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" <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
>>>>
>>>
>>
>> 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 jcholast at redhat.com in the commit 
message, so that people don't have to look me up.

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

>
> If not - attached is the new patch (correctly formatted).
>
> Thanks again!
>
>>
>> Honza
>>
>> --
>> Jan Cholasta
>>

Honza

-- 
Jan Cholasta