Dan Kenigsberg 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 \ Indeed, too bad pyflakes does not have this kind of syntax-sensitive check. (I know that "except Exception" is better. It's just not "really better". True improvement would come only by manual conversion of these catch-all clauses to something specific.) You are right that with no git around, the check does not fail (I missed that), but it adds another drop to the sea of build messages. Could you avoid that? I think Saggi mentioned another issue: if you `make check-local` on a dirty repository, you check someone else's code, not your own. How about doing git diff || git show ? 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
