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

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

2016-03-20 Thread Martin Basti 

Patches attached.
From e7a0809e0e202bd3729f2408fbc9855d9a7f10d4 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 11 Mar 2016 19:51:07 +0100
Subject: [PATCH 1/8] pylint: remove bare except

Bare except should not be used.
---
 client/ipa-client-install|  4 ++--
 install/tools/ipa-replica-manage |  2 +-
 install/tools/ipactl | 10 +-
 ipaclient/ipadiscovery.py|  2 +-
 ipalib/cli.py|  4 ++--
 ipalib/constants.py  |  4 ++--
 ipalib/plugins/dns.py|  4 ++--
 ipalib/plugins/hbactest.py   |  8 
 ipalib/plugins/stageuser.py  |  6 +++---
 ipalib/plugins/user.py   |  2 +-
 ipaplatform/base/services.py |  4 ++--
 ipapython/config.py  |  8 
 ipapython/ipautil.py |  4 ++--
 ipapython/log_manager.py |  2 +-
 ipapython/nsslib.py  | 17 ++---
 ipaserver/install/bindinstance.py|  4 ++--
 ipaserver/install/certs.py   |  2 +-
 ipaserver/install/dnskeysyncinstance.py  |  2 +-
 ipaserver/install/installutils.py|  4 ++--
 ipaserver/install/krbinstance.py |  6 +++---
 ipaserver/install/odsexporterinstance.py |  2 +-
 ipaserver/install/server/install.py  |  2 +-
 ipaserver/plugins/dogtag.py  |  2 +-
 ipatests/test_ipapython/test_ipautil.py  |  2 +-
 pylintrc |  1 -
 25 files changed, 51 insertions(+), 57 deletions(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index f42d877559365af3d8def3cab9b204cdb5eb919e..06905b1a8124218325db4bfe077da9edd2a87c01 100755
--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -2469,11 +2469,11 @@ def install(options, env, fstore, statestore):
 try:
 socket.inet_pton(socket.AF_INET, srv)
 is_ipaddr = True
-except:
+except socket.error:
 try:
 socket.inet_pton(socket.AF_INET6, srv)
 is_ipaddr = True
-except:
+except socket.error:
 is_ipaddr = False
 
 if is_ipaddr:
diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index 0497a0f0549b392216c654ab67d98cedbf122a4b..075e4293e1285a2bd17717a8ac8d9e42f624b1c0 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -597,7 +597,7 @@ def clean_dangling_ruvs(realm, host, options):
 conn = ipaldap.IPAdmin(master_cn, 636, cacert=CACERT)
 conn.do_simple_bind(bindpw=options.dirman_passwd)
 master_info['online'] = True
-except:
+except Exception:
 print("The server '{host}' appears to be offline."
   .format(host=master_cn))
 offlines.add(master_cn)
diff --git a/install/tools/ipactl b/install/tools/ipactl
index d27ada56556c58c42242cf0add11ef47d4440f56..fb1e890ea16af492e1dc4956d40a3fb6287d5837 100755
--- a/install/tools/ipactl
+++ b/install/tools/ipactl
@@ -343,13 +343,13 @@ def ipa_stop(options):
 try:
 print("Stopping %s Service" % svc)
 svchandle.stop(capture_output=False)
-except:
+except Exception:
 emit_err("Failed to stop %s Service" % svc)
 
 try:
 print("Stopping Directory Service")
 dirsrv.stop(capture_output=False)
-except:
+except Exception:
 raise IpactlError("Failed to stop Directory Service")
 
 # remove file with list of started services
@@ -383,7 +383,7 @@ def ipa_restart(options):
 emit_err("Shutting down")
 try:
 dirsrv.stop(capture_output=False)
-except:
+except Exception:
 pass
 if isinstance(e, IpactlError):
 # do not display any other error message
@@ -421,7 +421,7 @@ def ipa_restart(options):
 try:
 print("Stopping %s Service" % svc)
 svchandle.stop(capture_output=False)
-except:
+except Exception:
 emit_err("Failed to stop %s Service" % svc)
 
 try:
@@ -521,7 +521,7 @@ def ipa_status(options):
 print("%s Service: RUNNING" % svc)
 else:
 print("%s Service: STOPPED" % svc)
-except:
+except Exception:
 emit_err("Failed to get %s Service status" % svc)
 
 def main():
diff --git a/ipaclient/ipadiscovery.py b/ipaclient/ipadiscovery.py
index 1ba7c1a2d6ae071543b76812463d114de494c296..62c8aef9563b4cdbaf9454c1a20cc294e6b61704 100644
--- a/ipaclient/ipadiscovery.py
+++ b/ipaclient/ipadiscovery.py
@@ -129,7 +129,7 @@ class IPADiscovery(object):
 elif line.lower().startswith('search'):
 domains += [(d, 'search domain from /etc/resolv.conf') for
 d