[Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules

[Martin Basti]

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