On 16.08.2016 13:30, Stanislav Laznicka wrote:
On 08/16/2016 01:03 PM, Martin Basti wrote:


On 16.08.2016 12:06, Stanislav Laznicka wrote:
On 08/16/2016 08:36 AM, Stanislav Laznicka wrote:
On 08/02/2016 01:08 PM, Stanislav Laznicka wrote:
On 07/28/2016 10:57 AM, Martin Basti wrote:
Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
I chose ScriptError because it's able to change the return value of the script, which is necessary sometimes. RuntimeError, which may seem more suitable, would not be able to do that. However, I am open for ideas on which exception type to use.

2)
Can you use rather raise Exception(), instead of raise Exception

Sure, please see the modified attached patch.
3)
I really hate to print errors to STDOUT from modules and then just call exit(1) (duplicated evil), could you replace print('xxx') with raise AnException('xxx')
Did that, only kept those prints printing directly to stderr. Not sure if those should be changed as well.

Bumping for review/opinions.

Also a self-NACK, there were some sys imports that should not have been there anymore (thanks, mbasti). Attaching a fixed version.


You use RuntimeError in bindinstance.py, in other files you use ScriptError, is this RuntimeError on purpose there?

Martin^2

I used it there as there was one other function in the module that would raise it, too. Adding a patch that raises ScriptError in bindinstance.check_reverse_zones() as well if you think raising ScriptError here would be more appropriate.

ACK

Pushed to master: 5776f1e90000ccfc24689c99951864248ed01045

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to