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