Yaniv Bronhaim has posted comments on this change. Change subject: configfile utility for common config file editing operations. ......................................................................
Patch Set 1: (3 comments) http://gerrit.ovirt.org/#/c/27129/1/lib/vdsm/tool/configfile.py File lib/vdsm/tool/configfile.py: 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 and you don't need to have this condition Line 33: self.confstart = confstart Line 34: self.confend = confend Line 35: Line 36: def __enter__(self): Line 67: line = self.prefix + line Line 68: oldlines.append(line) Line 69: return oldlines, oldentries Line 70: Line 71: def __exit__(self, exec_ty, exec_val, tb): you can call it "flush" or "commit", and them assume you start new config, or have "clean" function that does what __enter__ does, instead of depending on __enter__ and __exit__ . its better practice not to depend on ctor\dtors with gc Line 72: Line 73: self.context = OUT Line 74: if exec_ty is None: Line 75: fd, tname = tempfile.mkstemp(dir=os.path.dirname(self.filename)) http://gerrit.ovirt.org/#/c/27129/1/tests/toolTests.py File tests/toolTests.py: 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 : with ConfigFile(params) as conf: ... ? if you create it before, i think it still holds it till the end of the function 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: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
