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

Mon Nov 12 14:04:23 UTC 2012

On 12.11.2012 14:14, Lynn Root wrote:
>
> ----- Original Message -----
>> 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.
>>
>
> Fixed.
>
>>>
>>>>
>>>> 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".
>
> Ah now I see - fixed.
>
>>
>>>
>>> If not - attached is the new patch (correctly formatted).
>>>
>>> Thanks again!
>>>
>>>>
>>>> Honza
>>>>
>>>> --
>>>> Jan Cholasta
>>>>
>>
>> Honza
>>
>> --
>> Jan Cholasta
>>
>
>
> Lynn Root
> Associate Software Engineer
> Red Hat
>

Thanks, ACK.

Honza

-- 
Jan Cholasta