[Freeipa-devel] [PATCH 0013] Remove user-unfriendly "u" character from error messages

[Martin Kosek]

On 09/06/2012 06:13 PM, Petr Viktorin wrote:
> On 09/05/2012 04:35 PM, Tomas Babej wrote:
>> On 09/05/2012 03:42 PM, Petr Viktorin wrote:
>>> On 09/05/2012 03:19 PM, Tomas Babej wrote:
>>>> Hi,
>>>>
>>>> User-unfriendly errors were caused by re-raising errors
>>>> from external python module netaddr.
>>>>
>>>> https://fedorahosted.org/freeipa/ticket/2588
>>>>
>>>> Tomas
>>>>
>>>>
>>>> _______________________________________________
>>>> Freeipa-devel mailing list
>>>> Freeipa-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>
>>>
>>> I don't agree with this approach. Raising another module's errors in
>>> our code is ugly, and will break if netaddr changes. The arguments to
>>> pass to the exceptions are undocumented (see
>>> http://packages.python.org/netaddr/api.html#custom-exceptions). The
>>> wording of error messages in libraries can usually change at any time,
>>> and is intended for developers, not end users.
>>>
>>> This should be either fixed upstream (unlikely, using the repr() of
>>> the argument is a sane thing to do at their side), or we should pass
>>> bytestrings to netaddr (a possible quick fix, not sure it it'll work),
>>> or, ideally, we should raise IPA's own errors.
>>>
>> Well, this particular fix wouldn't have broken anything, since it was
>> raising the same error that the except clause in which the raising
>> occured caught. However, I changed this to StandardError, since the
>> error message is extracted and packed into ValidationError during
>> further validation and therefore simple format message is suitable.
> 
> I know this is a minor issue and unlikely to cause problems, but it still
> should be fixed properly.
> 
> The patch assumes AddrFormatError takes only one argument, the message. In
> another case something like this might be a reasonable assumption, but having a
> prettier error message doesn't justify it.
> Taking free-form text from a library and fixing it up like this is also not
> maintainable. Again, it assumes too much about the library.
> 
> I won't ack this approach. Please consult someone else if you think it really
> is the best way.
> 
> Adding `addr = str(addr)` would work around the issue without indroducing
> assumptions about an external library.
> 
> 
> 
> Some technical issues with your patch, in case my "ideology" is incompatible
> with the project:
> 
> ValueError would be more appropriate than StandardError. We already raise it in
> similar situations in this method.
> 
> There is a case where your fix doesn't work: CheckedIPAddress(u'percent%sign').
> 
> Please adjust the test in test_dns_plugin that checks the error message.
> 

Just to make things clear - this patch and this thread was superseded by Lynn's
patch 0001.

Martin