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

[Lynn Root]

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.

> 
> 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?

If not - attached is the new patch (correctly formatted).

Thanks again!

> 
> Honza
> 
> --
> Jan Cholasta
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-lroot-0001-5-Switch-r-specifiers-to-s-in-Public-errors.patch
Type: text/x-patch
Size: 13421 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121112/593620ec/attachment.bin>