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

[Stanislav Laznicka]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0048-5-Remove-sys.exit-from-install-modules-and-scripts.patch
Type: text/x-patch
Size: 44093 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160816/4d7d1b76/attachment.bin>