Re: [Freeipa-devel] [PATCHES 0442-0449] Pylint: sunday code cleanup

2016-03-22 Thread Martin Basti



On 21.03.2016 16:52, Petr Spacek wrote:

On 21.3.2016 13:50, Lukas Slebodnik wrote:

On (21/03/16 12:30), Martin Basti wrote:

On 21.03.2016 10:33, Christian Heimes wrote:

On 2016-03-21 10:29, Petr Spacek wrote:

On 20.3.2016 21:56, Martin Basti wrote:

Patches attached.

I do not really like
freeipa-mbasti-0442-pylint-remove-bare-except
because it replaces most of

try: ... except:

with

try: ... except Exception:


which AFAIK does not add any value. It would be better to replace Exception
with more specific exception so the code raises an error instead of continuing
when something really unexpected happens.

It adds some value. A bare except also excepts signals like
KeyboardInterrupt and SystemExit. except Exception doesn't block these
exceptions.

But yes, more specific exceptions are better.

Christian





'except Exception' is another pylint check :D

I replaced bare except with a particular exception in cases where it was
clear. For other occurrences of bare except it covers too much Exception
types, so catch Exception is more sensible, or I need crystal ball to detect
what kind of exceptions can be raised there.


Agree.

It can be changed to more specific exceptions type of Exception in future.
This change is less risky.

pylint passed on fedora {23, 24, rawhide}

ACK

Okay, it makes sense.


master:
* 491447cc5ab8c5eff2be57d609201cefb79f7053 pylint: remove bare except
* e93e89e1ae27e4f0ef23001f6c1247c45695ae24 Pylint: fix definition of 
global variables
* 5add0f94cf9253a72224ccaf5be38540468ea589 Pylint: enable 
pointless-except check

* d46cd5d956d1c03b863bf90d0fd0ff4870448183 Pylint: enable reimported check
* 195e50b93b63e4f30ce83dbcfef278727d48aea2 Pylint: use list 
comprehension instead of iteration
* b66028af1815fbf7666b82ebeaa81ad56996a74f Pylint: import max one module 
per line
* da0318d4d7dd369be136449e686b6fb46d0cc5d8 Pylint: remove 
unnecessary-semicolon

* 4a396dd68b1bc6cc68765f502f7e952a087064a8 Pylint: enable invalid-name check

--
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


Re: [Freeipa-devel] [PATCHES 0442-0449] Pylint: sunday code cleanup

2016-03-21 Thread Petr Spacek
On 21.3.2016 13:50, Lukas Slebodnik wrote:
> On (21/03/16 12:30), Martin Basti wrote:
>> On 21.03.2016 10:33, Christian Heimes wrote:
>>> On 2016-03-21 10:29, Petr Spacek wrote:
 On 20.3.2016 21:56, Martin Basti wrote:
> Patches attached.
 I do not really like
 freeipa-mbasti-0442-pylint-remove-bare-except
 because it replaces most of

 try: ... except:

 with

 try: ... except Exception:


 which AFAIK does not add any value. It would be better to replace Exception
 with more specific exception so the code raises an error instead of 
 continuing
 when something really unexpected happens.
>>> It adds some value. A bare except also excepts signals like
>>> KeyboardInterrupt and SystemExit. except Exception doesn't block these
>>> exceptions.
>>>
>>> But yes, more specific exceptions are better.
>>>
>>> Christian
>>>
>>>
>>>
>>>
>> 'except Exception' is another pylint check :D
>>
>> I replaced bare except with a particular exception in cases where it was
>> clear. For other occurrences of bare except it covers too much Exception
>> types, so catch Exception is more sensible, or I need crystal ball to detect
>> what kind of exceptions can be raised there.
>>
> Agree.
> 
> It can be changed to more specific exceptions type of Exception in future.
> This change is less risky.
> 
> pylint passed on fedora {23, 24, rawhide}
> 
> ACK

Okay, it makes sense.

-- 
Petr^2 Spacek

-- 
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


Re: [Freeipa-devel] [PATCHES 0442-0449] Pylint: sunday code cleanup

2016-03-21 Thread Lukas Slebodnik
On (21/03/16 12:30), Martin Basti wrote:
>On 21.03.2016 10:33, Christian Heimes wrote:
>>On 2016-03-21 10:29, Petr Spacek wrote:
>>>On 20.3.2016 21:56, Martin Basti wrote:
Patches attached.
>>>I do not really like
>>>freeipa-mbasti-0442-pylint-remove-bare-except
>>>because it replaces most of
>>>
>>>try: ... except:
>>>
>>>with
>>>
>>>try: ... except Exception:
>>>
>>>
>>>which AFAIK does not add any value. It would be better to replace Exception
>>>with more specific exception so the code raises an error instead of 
>>>continuing
>>>when something really unexpected happens.
>>It adds some value. A bare except also excepts signals like
>>KeyboardInterrupt and SystemExit. except Exception doesn't block these
>>exceptions.
>>
>>But yes, more specific exceptions are better.
>>
>>Christian
>>
>>
>>
>>
>'except Exception' is another pylint check :D
>
>I replaced bare except with a particular exception in cases where it was
>clear. For other occurrences of bare except it covers too much Exception
>types, so catch Exception is more sensible, or I need crystal ball to detect
>what kind of exceptions can be raised there.
>
Agree.

It can be changed to more specific exceptions type of Exception in future.
This change is less risky.

pylint passed on fedora {23, 24, rawhide}

ACK

LS

-- 
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


Re: [Freeipa-devel] [PATCHES 0442-0449] Pylint: sunday code cleanup

2016-03-21 Thread Martin Basti



On 21.03.2016 10:33, Christian Heimes wrote:

On 2016-03-21 10:29, Petr Spacek wrote:

On 20.3.2016 21:56, Martin Basti wrote:

Patches attached.

I do not really like
freeipa-mbasti-0442-pylint-remove-bare-except
because it replaces most of

try: ... except:

with

try: ... except Exception:


which AFAIK does not add any value. It would be better to replace Exception
with more specific exception so the code raises an error instead of continuing
when something really unexpected happens.

It adds some value. A bare except also excepts signals like
KeyboardInterrupt and SystemExit. except Exception doesn't block these
exceptions.

But yes, more specific exceptions are better.

Christian





'except Exception' is another pylint check :D

I replaced bare except with a particular exception in cases where it was 
clear. For other occurrences of bare except it covers too much Exception 
types, so catch Exception is more sensible, or I need crystal ball to 
detect what kind of exceptions can be raised there.


Martin^2
-- 
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

Re: [Freeipa-devel] [PATCHES 0442-0449] Pylint: sunday code cleanup

2016-03-21 Thread Christian Heimes
On 2016-03-21 10:29, Petr Spacek wrote:
> On 20.3.2016 21:56, Martin Basti wrote:
>> Patches attached.
> 
> I do not really like
> freeipa-mbasti-0442-pylint-remove-bare-except
> because it replaces most of
> 
> try: ... except:
> 
> with
> 
> try: ... except Exception:
> 
> 
> which AFAIK does not add any value. It would be better to replace Exception
> with more specific exception so the code raises an error instead of continuing
> when something really unexpected happens.

It adds some value. A bare except also excepts signals like
KeyboardInterrupt and SystemExit. except Exception doesn't block these
exceptions.

But yes, more specific exceptions are better.

Christian




signature.asc
Description: OpenPGP digital signature
-- 
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

Re: [Freeipa-devel] [PATCHES 0442-0449] Pylint: sunday code cleanup

2016-03-21 Thread Petr Spacek
On 20.3.2016 21:56, Martin Basti wrote:
> Patches attached.

I do not really like
freeipa-mbasti-0442-pylint-remove-bare-except
because it replaces most of

try: ... except:

with

try: ... except Exception:


which AFAIK does not add any value. It would be better to replace Exception
with more specific exception so the code raises an error instead of continuing
when something really unexpected happens.


Other patches look sensible to me.

-- 
Petr^2 Spacek

-- 
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