Change in vdsm[master]: build: Forbid bare except:

smizrahi at redhat.com smizrahi at redhat.com
Sun Dec 22 15:59:41 UTC 2013


Saggi Mizrahi has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................


Patch Set 3:

(1 comment)

....................................................
File Makefile.am
Line 78: SKIP_PYFLAKES_ERR = "\./vdsm/storage/lvm\.py.*: list comprehension redefines \
Line 79: 	'lv' from line .*"
Line 80: 
Line 81: check-local:
Line 82: 	if git show | egrep '^\+\s+except:\s*(#.+)?$$'; then \
The reason we accept the will of the pep8 gods is so that we don't argue about what is the right thing to do.

pep8 is the recommended way to format python code and it prevents bike-shedding over spaces\lines and other unimportant things. If pep8 accepts it than it's good enough. End of story.

doing except Exception is better as it doesn't catch InterruptError and other low level errors. On the other hand this doesn't really matter in VDSM as we have a handler for interrupts.

In any case I would much rather have this be a bit more reliable.
pep8 has a sort of a plugin system so you can use it to find bare excepts.

a quick grep shows that there are 173 cases of "except:"
Some are legitimate.

For instance in remoteFileHandler:PoolHandler.__init__

Also, I never understood why "except:" or "except Exception:" is that bad.

In c it's common practice to do:
if(some_opration() < 0) {
  log()
  goto cleanup
}

A lot of the time you don't handle specific errors differently and you just want to handle *any* failure. I'll wager that this is what you want *most* of the time.

The only way to actually prevent bare excepts or except Exception is to make errors part of the interface all throughout the system.

This is nearly impossible in python as the compiler will not check that you remembered to check for that new error all through the stack.

for example in:
remoteFileHandler:PoolHandler.stop
I don't want to care what exception os.kill might return.
All I care is that it failed and to ignore it. Why do I need to check if it's an OSError or IOError or whatever.

It could have been changed to except Exception for added verbosity and to ignore BaseException for whatever reason.
Line 83: 		echo "error: empty "except:" is forbidden!"; \
Line 84: 		exit 1; \
Line 85: 	fi
Line 86: 	find . -path './.git' -prune -type f -o \


-- 
To view, visit http://gerrit.ovirt.org/22456
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5b7f33b6eb1c90610b3a5d053ba65ce3fc6b74fa
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list