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

[Petr Viktorin]

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.

-- 
Petr³