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.
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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
