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