On Apr 20, 2011, at 10:32 AM, Rob Crittenden wrote: ... >>> Seems to work as advertised, I just have a couple of requests: >>> >>> - Some of the comments are really long, can you limit to ~75 chars per line? >>> - In this code block: >>> >>> for r in results: >>> direct.append(r) >>> try: >>> indirect.remove(r.lower()) >>> except ValueError: >>> pass >>> >>> We should log if a ValueError is thrown, this would mean something is >>> really wrong. Can you import logging in the file and replace pass with >>> something like: >>> >>> logging.info('Failed to remove indirect entry %s from %s' % r, entry_dn) >>> >>> I wonder if we should raise the ValueError too. This means that something >>> went very wrong. >> >> Yes I agree that we should raise the error. >> >> Here is the patch with the requested changes: >> >> * Fixed width to be PEP8 compliant >> * import logging >> * Added logging line in exception >> * Raise ValueError if we blow up during indirect removal. >> > > Fixes 1-3 look good. I think for the exception you want: > > except ValueError, e: > logging .... > raise e > > A ValueError won't get returned over XML-RPC but the full backtrace will be > logged on the server side. The end-user will get a 903 (Internal error > raised). > > rob
Ok adjusted patch attached to correct for the exception raised.
_______________________________________________ Freeipa-devel mailing list Freeipaemail@example.com https://www.redhat.com/mailman/listinfo/freeipa-devel