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 <nsof...@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
