mooli tayer has posted comments on this change. Change subject: configfile utility for common config file editing operations. ......................................................................
Patch Set 1: (10 comments) http://gerrit.ovirt.org/#/c/27129/1/lib/vdsm/tool/configfile.py File lib/vdsm/tool/configfile.py: Line 26: class ConfigFile(object): Line 27: Line 28: def __init__(self, filename, confstart=None, confend=None): Line 29: self.filename = filename Line 30: self.context = OUT > Missing definition Fixed Line 31: if confstart is None or confend is None: Line 32: raise RuntimeError("argument missing") Line 33: self.confstart = confstart Line 34: self.confend = confend Line 28: def __init__(self, filename, confstart=None, confend=None): Line 29: self.filename = filename Line 30: self.context = OUT Line 31: if confstart is None or confend is None: Line 32: raise RuntimeError("argument missing") > so why is it valid to call it without those arguments? don't put defaults a Right! Line 33: self.confstart = confstart Line 34: self.confend = confend Line 35: Line 36: def __enter__(self): Line 48: def getOldContent(self): Line 49: confpat = re.compile(r'^\s*([^=\s]*)\s*=') Line 50: oldlines = [] Line 51: oldentries = set() Line 52: with open(self.filename, 'r') as f: > handle exceptions here.. Why not let the exception go up? Line 53: for line in f: Line 54: if self.remove: Line 55: if self.rmstate == BEFORE and\ Line 56: line.startswith(self.confstart): Line 51: oldentries = set() Line 52: with open(self.filename, 'r') as f: Line 53: for line in f: Line 54: if self.remove: Line 55: if self.rmstate == BEFORE and\ > Use of () is preferable to \ replaced Line 56: line.startswith(self.confstart): Line 57: self.rmstate = WITHIN Line 58: elif self.rmstate == WITHIN and\ Line 59: line.startswith(self.confend): Line 74: if exec_ty is None: Line 75: fd, tname = tempfile.mkstemp(dir=os.path.dirname(self.filename)) Line 76: oldlines, oldentries = self.getOldContent() Line 77: with os.fdopen(fd, 'w') as f: Line 78: if self.section: > Split to smaller helper methods Done Line 79: f.write(self.confstart) Line 80: f.write(self.section) Line 81: f.write(self.confend) Line 82: f.writelines(oldlines) Line 101: """ Line 102: if self.context == IN: Line 103: self.entries[key] = val Line 104: else: Line 105: raise RuntimeError("Internal error") > print "'addEntry' must be run in Config context", move clear than Internal Done Line 106: Line 107: def prependSection(self, section): Line 108: """ Line 109: add 'section' in the beginning of the file. Line 109: add 'section' in the beginning of the file. Line 110: section is wrapped with confstart and confend. Line 111: Only one section is currently allowed. Line 112: """ Line 113: if self.context == IN: > Could be a decorator Not sure I follow Line 114: self.section = section Line 115: else: Line 116: raise RuntimeError("Internal error") Line 117: Line 119: """ Line 120: prefix each line originaly included in the file (not including Line 121: 'prependedSection' lines) with 'prefix'. Line 122: """ Line 123: if self.context == IN: > context probably better off as a boolean agree Line 124: self.prefix = prefix Line 125: else: Line 126: raise RuntimeError("Internal error") Line 127: http://gerrit.ovirt.org/#/c/27129/1/tests/toolTests.py File tests/toolTests.py: Line 178: Line 179: Line 180: class ConfigFileTests(TestCase): Line 181: def setUp(self): Line 182: fd, self.tname = tempfile.mkstemp() > close the fd or you'll cause an fd leak Done Line 183: Line 184: def tearDown(self): Line 185: os.remove(self.tname) Line 186: Line 196: confstart="# start conf-3.4.4\n", Line 197: confend="# end conf-3.4.4\n") Line 198: with conf_file as conf: Line 199: conf.addEntry("key3", "val3") Line 200: conf.addEntry("key2", "val3") > does it destroy the object at the end of this context? don't you need to do is that wrong? I guess I will do anyway: with ConfigFile(self.tname, confstart="# start conf-3.4.4\n", confend="# end conf-3.4.4\n") as conf: conf.addEntry("key3", "val3") conf.addEntry("key2", "val3") Line 201: with open(self.tname, 'r') as f: Line 202: Line 203: self.assertEqual(f.read(), "key1=val1\n" Line 204: " key2 =val2\n" -- To view, visit http://gerrit.ovirt.org/27129 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If8c7ae2b562650e403fc39024f3531d44b2b9c4f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: mooli tayer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: mooli tayer <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
