Saggi Mizrahi has posted comments on this change. Change subject: configfile utility for common config file editing operations. ......................................................................
Patch Set 1: Code-Review-1 (7 comments) Whats wrong with ConfigParser[1] ? [1] https://docs.python.org/2/library/configparser.html 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 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 Also, incorrect values throw ValueError Line 33: self.confstart = confstart Line 34: self.confend = confend Line 35: Line 36: def __enter__(self): 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 \ 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 writeSection() writeEntries() etc.. Line 79: f.write(self.confstart) Line 80: f.write(self.section) Line 81: f.write(self.confend) Line 82: f.writelines(oldlines) 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 @context(IN) def foo(): 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 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 Line 183: Line 184: def tearDown(self): Line 185: os.remove(self.tname) Line 186: -- 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: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
