On 11/12/2012 03:04 PM, Jan Cholasta wrote: > 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" <jchol...@redhat.com> >>>>> To: "Lynn Root" <lr...@redhat.com> >>>>> Cc: freeipa-devel@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" <mko...@redhat.com> >>>>>> To: "Jan Cholasta" <jchol...@redhat.com> >>>>>> Cc: "Lynn Root" <lr...@redhat.com>, freeipa-devel@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" <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. >>>>>> >>>>>> 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 jchol...@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 >
I verified the patch is still OK and pushed it to master. I also added Lynn to the Contributors.txt file, she has earned the right to be listed there :-) Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
