Change in vdsm[master]: configfile utility for common config file editing operations.
smizrahi at redhat.com
smizrahi at redhat.com
Tue Apr 29 11:40:26 UTC 2014
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 <mtayer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list