Alon Bar-Lev has posted comments on this change. Change subject: engine: refactor: add Model attribute for help tagging ......................................................................
Patch Set 12: (11 comments) http://gerrit.ovirt.org/#/c/21052/12/build/helptag.py File build/helptag.py: Line 60: """ Line 61: look for help tags in the source code. Line 62: """ Line 63: tags = {} Line 64: buffer = "" please remove Line 65: Line 66: if filename.endswith('.java') and os.path.isfile(filename): Line 67: with open(filename, 'r') as f: Line 68: content = f.read() Line 66: if filename.endswith('.java') and os.path.isfile(filename): Line 67: with open(filename, 'r') as f: Line 68: content = f.read() Line 69: for block in content.split(';'): Line 70: name = "" I would have put None here... Line 71: comment = "" Line 72: for m in __RE_ANNOTATION.findall(block): Line 73: if m[0] == "Name": Line 74: name = m[1] http://gerrit.ovirt.org/#/c/21052/12/build/helptag_checker.py File build/helptag_checker.py: Line 42: \s* Line 43: \)\s*; Line 44: .* Line 45: """ Line 46: ) ok... so I do not understand why there are to expressions above... it can be one... just like in the other program. also to understand if there is quotes or not you can use: 'x\((?P<content>("(?P<quoted>[^"]*)")|.*)\)' then quoted will be non None if quoted, else checkout content. Line 47: Line 48: __RE_QUOTED_STRING = re.compile( Line 49: flags=re.VERBOSE, Line 50: pattern=r'"[^"]*"' Line 61: filename = os.path.join(parent, fname) Line 62: if filename.endswith('.java') and os.path.isfile(filename): Line 63: f = open(filename) Line 64: lines = f.readlines() Line 65: f.close() please use with clause Line 66: yield (lines, fname) Line 67: Line 68: Line 69: def findVariableHashNames(sourcedir): Line 74: if m: Line 75: hashname = m.group("hashname") Line 76: m2 = __RE_QUOTED_STRING.match(hashname) Line 77: if not m2: Line 78: message += ("bad line: %s" % line) there is no need for () Line 79: Line 80: if message: Line 81: warning("\n%s\n%s\n" % (fname, message)) Line 82: Line 77: if not m2: Line 78: message += ("bad line: %s" % line) Line 79: Line 80: if message: Line 81: warning("\n%s\n%s\n" % (fname, message)) a strange warning that starts with new line... maybe better to call warning for each line instead of creating long message? Line 82: Line 83: Line 84: def findHashNamesWithoutHelpTag(sourcedir): Line 85: for (lines, fname) in walkSource(sourcedir): Line 87: for i in range(0, len(lines)): Line 88: m = __RE_SETHASHNAME.match(lines[i]) Line 89: if m: Line 90: hashname = m.group("hashname").replace('"', '') \ Line 91: .replace('-', '_') try the following syntax: hashname = m.group(...).replace( '''', '' ).replace( '' '' ) and drop the \ Line 92: if lines[i-1].find(hashname) == -1 or \ Line 93: lines[i-1].find('setHelpTag') == -1: Line 94: message += ( Line 95: "helptag hashname mismatch:\n%s%s" Line 89: if m: Line 90: hashname = m.group("hashname").replace('"', '') \ Line 91: .replace('-', '_') Line 92: if lines[i-1].find(hashname) == -1 or \ Line 93: lines[i-1].find('setHelpTag') == -1: try the following syntax: if ( statement or statement ): Line 94: message += ( Line 95: "helptag hashname mismatch:\n%s%s" Line 96: % (lines[i-1], lines[i]) Line 97: ) Line 96: % (lines[i-1], lines[i]) Line 97: ) Line 98: Line 99: if message: Line 100: warning("\n%s\n%s\n" % (fname, message)) same here... you can call warning each no? Line 101: Line 102: Line 103: def findDuplicateHelpTagCalls(sourcedir): Line 104: Line 110: helptag = m.group("helptag") Line 111: if helptag in tags: Line 112: tags[helptag].append(fname) Line 113: else: Line 114: tags[helptag] = [fname] tags.setdefault(helptag, []).append(fname) Line 115: Line 116: for helptag in tags.keys(): Line 117: files = tags[helptag] Line 118: if (len(files) > 1): Line 119: warning("duplicate helptag call:\n%s\n%s\n\n" % (helptag, files)) Line 120: Line 121: Line 122: def warning(s): Line 123: sys.stdout.write("WARNING: %s" % s) stderr is better I think Line 124: Line 125: Line 126: def main(): Line 127: parser = argparse.ArgumentParser( -- To view, visit http://gerrit.ovirt.org/21052 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4074fcc2ecfcbdd2ea6c0855d92f2aa4bd26a5b Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
