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

[Lynn Root]

----- 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-lroot-0001-6-Switch-r-specifiers-to-s-in-Public-errors.patch
Type: text/x-patch
Size: 14266 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121112/91b10630/attachment.bin>