Change in vdsm[master]: v2v: introduction to OutputParser
nsoffer at redhat.com
nsoffer at redhat.com
Mon Mar 30 15:20:18 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: v2v: introduction to OutputParser
......................................................................
Patch Set 1: Code-Review+1
(3 comments)
Looks good, can be improved a little bit by separating iteration and parsing code.
https://gerrit.ovirt.org/#/c/39360/1/tests/v2vTests.py
File tests/v2vTests.py:
Line 24
Line 25
Line 26
Line 27
Line 28
Why delete the previous tests?
https://gerrit.ovirt.org/#/c/39360/1/vdsm/v2v.py
File vdsm/v2v.py:
Line 91: description, current_disk, disk_count = self._parse_line(line)
Line 92: yield ImportProgress(int(current_disk), int(disk_count),
Line 93: description)
Line 94: for progress in self._parse_progress(stream):
Line 95: yield DiskProgress(progress)
If you use my suggestions bellow:
for chunk in self._iter_progress(stream):
progress = self._parse_progress(chunk)
yield DiskProgress(progress)
if progress == 100:
break
This keep all the flow logic in one place, and keep the parsing code away.
Line 96:
Line 97: def _parse_line(self, line):
Line 98: m = self.COPY_DISK_RE.match(line)
Line 99: if m is None:
Line 116: try:
Line 117: progress = int(m.group(1))
Line 118: except ValueError:
Line 119: raise OutputParserError('error parsing progress regex: %r'
Line 120: % m.groups)
This is little bit too long, and is doing two things - extracting chunks terminated by "\r", and parsing the content of the chunks.
This block can be extracted to _parse_progress() (like _parse_line).
Then the rest can renamed to _iter_progress(). Will be easier to follow.
Line 121: yield progress
Line 122:
Line 123:
Line 124: def _mem_to_mib(size, unit):
--
To view, visit https://gerrit.ovirt.org/39360
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54f3d610d704d0b922e38073edcc813cbc27fe83
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer 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