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 \